impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gabor Kaszab (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5511: Add process start time to debug web page
Date Mon, 24 Jul 2017 15:13:19 GMT
Gabor Kaszab has posted comments on this change.

Change subject: IMPALA-5511: Add process start time to debug web page
......................................................................


Patch Set 8:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/7363/8/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:

PS8, Line 53: const string CATALOG_SERVER_START_TIME =
            :     "process-start-time";
> nit: one line?
Done


PS8, Line 198: start_time_metric_->set_value(TimestampValue::LocalTime().ToString());
> since we're calling this 'process start time' I would put its initializatio
Done


http://gerrit.cloudera.org:8080/#/c/7363/8/be/src/catalog/catalog-server.h
File be/src/catalog/catalog-server.h:

Line 203:   /// Function is meant to be called from the CatalogServer constructor in order
to register
> nit: long line
Done


PS8, Line 203: Function is meant to be called from the CatalogServer constructor
> Don't say where this should be called from (that's the caller's decision!) 
Done


http://gerrit.cloudera.org:8080/#/c/7363/8/be/src/util/default-path-handlers.cc
File be/src/util/default-path-handlers.cc:

PS8, Line 214: string GetProcessStartTime(MetricGroup* metric_group) {
             :   string error_text = "Error: Process start time can't be retrieved.";
             :   if (metric_group == nullptr) {
             :     return error_text;
             :   }
             : 
             :   Metric* start_time_metric = metric_group->GetMetricByName("process-start-time");
             :   if (start_time_metric == nullptr) {
             :     return error_text;
             :   }
             : 
             :   StringProperty* start_time_property = reinterpret_cast<StringProperty*>(start_time_metric);
             :   if (start_time_property == nullptr) {
             :     return error_text;
             :   }
             : 
             :   return start_time_property->value();
             : }
> this shouldn't be necessary: we should ensure the metric is always created,
For the sake of robustness I would keep these checks.  I'll drop the second one (start_time_metric
== nullptr check) as it would be caught at the next check.
What do you think?


http://gerrit.cloudera.org:8080/#/c/7363/8/be/src/util/metrics.cc
File be/src/util/metrics.cc:

Line 230: 
> nit: remove blank line
Done


http://gerrit.cloudera.org:8080/#/c/7363/8/www/root.tmpl
File www/root.tmpl:

PS8, Line 38: <h2>Process Start Time</h2>
            :   <pre>{{process_start_time}}</pre>
> can you move this above Hardware Info? I think it's useful to see this near
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I05ae2f80835b1b0e4bc3b38731778ba0871338a4
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Gabor Kaszab <gaborkaszab@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <gaborkaszab@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <laszlo.gaal@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message