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 10: (5 comments) http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/tools/tool_action_master.cc@149 PS10, Line 149: std::move no need for 'std::move' here since the return value of a function is already a temporary (technically I think it's called a "prvalue", and so the rvalue reference argument to emplace_back will already bind to it directly, see https://en.cppreference.com/w/cpp/language/value_category ) http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/tools/tool_action_tserver.cc File src/kudu/tools/tool_action_tserver.cc: http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/tools/tool_action_tserver.cc@148 PS10, Line 148: std::move same http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/util/CMakeLists.txt File src/kudu/util/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/util/CMakeLists.txt@259 PS10, Line 259: wire_protocol_proto see comment elsewhere: this is a circular reference between modules that we don't want http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/util/pb_util.h File src/kudu/util/pb_util.h: http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/util/pb_util.h@39 PS10, Line 39: } // namespace kudu I don't think this belongs in util/pb_util.h. util/ is meant to be very generic utility code that could be shared between projects, so a backward dependency up into kudu server specific stuff like this isn't clean. (In fact, Impala pulls in the util/ directory since it shares our krpc stack) Can you move this utility function somewhere else like common/wire_protocol.h since the PB is in wire_protocol.proto in that same package? http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/util/pb_util.h@124 PS10, Line 124: std::string ParseStartTime(const ServerRegistrationPB& reg); nit: better to name this "StartTimeToString" rather than "Parse", since imo "parse" implies going _from_ a string format rather thna _to_ one -- 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: 10 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: Fri, 15 Feb 2019 06:06:52 +0000 Gerrit-HasComments: Yes
