Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12304 )
Change subject: [server] Add start_time in server description ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/common/wire_protocol.proto File src/kudu/common/wire_protocol.proto: http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/common/wire_protocol.proto@96 PS4, Line 96: optional int64 start_time = 5; can you add a comment which indicates the unit and reference point? ie "seconds since the epoch" or "milliseconds since the epoch" or something? http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/master/master_path_handlers.cc@138 PS4, Line 138: (time_t) nit: we prefer using C++ syntax for casts: static_cast<time_t>(...) http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/master/master_path_handlers.cc@479 PS4, Line 479: // Convert epoch time to localtime. need to handle the case where 'reg.has_start_time()' is false (eg post upgrade) to not set it in the output JSON http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/master/master_path_handlers.cc@663 PS4, Line 663: jw.String("start_time"); same http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/tools/tool_action_master.cc@152 PS4, Line 152: StringAppendStrftime(&time_str, "%Y-%m-%d %H:%M:%S %Z", I think you need to wrap this block in something like: if (registration().has_start_time()) { ... } else { time_str = "<unknown>"; } since during a rolling upgrade from an earlier release we won't have a time set, and we don't want to show it as 1970-01-01 http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/tools/tool_action_master.cc@154 PS4, Line 154: values.emplace_back(time_str); nit: can std::move() this http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/tools/tool_action_tserver.cc File src/kudu/tools/tool_action_tserver.cc: http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/tools/tool_action_tserver.cc@150 PS4, Line 150: string time_str; same comment as in tool_action_master.cc http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/tools/tool_action_tserver.cc@153 PS4, Line 153: values.emplace_back(time_str); nit: std::move http://gerrit.cloudera.org:8080/#/c/12304/4/www/tablet-servers.mustache File www/tablet-servers.mustache: http://gerrit.cloudera.org:8080/#/c/12304/4/www/tablet-servers.mustache@58 PS4, Line 58: <td><pre><code>{{registration}} start_time: {{start_time}}</code></pre></td> maybe breaking out the start time to a separate column would be good? -- To view, visit http://gerrit.cloudera.org:8080/12304 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I041467014499ad00c07398ad8f61c6d6ca1ceeca Gerrit-Change-Number: 12304 Gerrit-PatchSet: 4 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Tue, 12 Feb 2019 05:35:20 +0000 Gerrit-HasComments: Yes
