Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/12304 )
Change subject: [server] Add start_time in server description ...................................................................... Patch Set 5: (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: > can you add a comment which indicates the unit and reference point? ie "sec Done 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: desc->l > nit: we prefer using C++ syntax for casts: static_cast<time_t>(...) Done http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/master/master_path_handlers.cc@479 PS4, Line 479: } > need to handle the case where 'reg.has_start_time()' is false (eg post upgr Done http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/master/master_path_handlers.cc@663 PS4, Line 663: } > same Done 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: values.emplace_back(std::move(pb_util::ParseStartTime(master.registration()))); > I think you need to wrap this block in something like: Done http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/tools/tool_action_master.cc@154 PS4, Line 154: } else { > nit: can std::move() this Done 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: for (const auto& server : servers) { > same comment as in tool_action_master.cc Done http://gerrit.cloudera.org:8080/#/c/12304/4/src/kudu/tools/tool_action_tserver.cc@153 PS4, Line 153: } else { > nit: std::move Done 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>{{time_since_hb}}</td> > maybe breaking out the start time to a separate column would be good? Done -- 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: 5 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 12 Feb 2019 15:58:08 +0000 Gerrit-HasComments: Yes
