impala-reviews mailing list archives

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

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


Patch Set 9:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/7363/9/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

PS9, Line 240:   RegisterMetrics(metrics);
             : }
             : 
             : void Statestore::RegisterMetrics(MetricGroup* metrics) {
you can undo this change, I think it's reasonable to have that code in-line in the c'tor for
now.


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

Line 27:   process_start_time_ = metric_group->AddProperty<string>(PROCESS_START_TIME_METRIC_NAME,
"");
long line


PS9, Line 30: void CommonMetrics::InitCommonMetrics() {
            :   // TODO: IMPALA-5599: Clean up non-TIMESTAMP usages of TimestampValue
            :   process_start_time_->set_value(TimestampValue::LocalTime().ToString());
            : }
let's combine these two methods into one (since they should always be called together). InitCommonMetrics()
is a fine name for the combined method.


http://gerrit.cloudera.org:8080/#/c/7363/9/be/src/util/common-metrics.h
File be/src/util/common-metrics.h:

PS9, Line 22: //class MetricGroup;
            : //class StringProperty;
remove


PS9, Line 27: of
for


PS9, Line 30: process_start_time_
PROCESS_START_TIME


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

PS9, 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");
             :   StringProperty* start_time_property = reinterpret_cast<StringProperty*>(start_time_metric);
             :   if (start_time_property == nullptr) {
             :     return error_text;
             :   }
             : 
             :   return start_time_property->value();
             : }
I think this can be replaced with (CommonMetrics::PROCESS_START_TIME != nullptr)


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

PS9, Line 22: #include "string.h"
remove


-- 
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: 9
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