Dinesh Bhat has posted comments on this change. Change subject: [tools] Implement a manual leader_step_down for a tablet ......................................................................
Patch Set 9: (5 comments) > (5 comments) > > There is one extra instance of "master_addresses" string literal > and corresponding description in tool_action_test.cc. Could you > updated that to use the common constant as well? TFTR, that instance was missing since I hadn't rebased on top of your latest tool change I think, rebased and updated now. http://gerrit.cloudera.org:8080/#/c/4533/9/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: PS9, Line 102: hostname:port > Nit: do you want to add that the 'port' part could be omitted if the master I guess so, although I would like to punt this into another patch after making sure all the RPCs used by the toolset comply to that theory. PS9, Line 104: I > Nit: Is capitalization of 'i' intentional? Sort of. I think we could be bit more consistent here; this was more in the spirit of 'Tablet Server, Kudu Master'.. and so on although comparison isn't really apple-to-apple. We have also used 'block identifier' to counter my argument here. http://gerrit.cloudera.org:8080/#/c/4533/9/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: PS9, Line 81: extern > Does 'extern' have to be present due to some compilation issues? I would e Fixed the ordering. About 'extern', I remember looking up cpp guideline for this, since I couldn't find anything, I followed the existing code's policy. Besides, explicit keyword provides a really strong readability. http://gerrit.cloudera.org:8080/#/c/4533/9/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: PS9, Line 175: string tablet_id > nit: const string& tablet_id ? Done Line 246: "to step down") > nit: parameter/line continuation shift is 4 spaces. I am not clear about this. Guideline seems to say : "start the arguments on a new line indented by four spaces and continue at that 4 space indent". This isn't necessarily starting the argument on a newline. I remember following what other files have followed here, for eg take a look at tool_action_*.cc. If needed correction, I can fix them all in this change or a different change. -- To view, visit http://gerrit.cloudera.org:8080/4533 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia046a28a2008f4f5d1e955f57752a32a1ddc5ab8 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
