Dinesh Bhat has posted comments on this change. Change subject: tool: port kudu-admin to 'kudu cluster' ......................................................................
Patch Set 4: (6 comments) Consider +1 from me, except one Qn about REMOVE_SERVER below. http://gerrit.cloudera.org:8080/#/c/4180/3/src/kudu/tools/tool_action_cluster.cc File src/kudu/tools/tool_action_cluster.cc: Line 293: unique_ptr<Action> add_server = > But they are already alphabetically sorted, aren't they? Or do you mean I s I missed that it's already sorted in BuildHelp, never mind then. http://gerrit.cloudera.org:8080/#/c/4180/4/src/kudu/tools/tool_action_cluster.cc File src/kudu/tools/tool_action_cluster.cc: Line 91: const char* const kTabletIdArg = "tablet_id"; nice, i can consolidate in kudu-test-tool as well like this, but why not const string ? PS4, Line 117: tablet_raw Bear with a basic Qn: what prevents us from using unique_ptr directly instead of using raw pointer first and then re-assigning ? PS4, Line 177: kudu some const string again ? or GetBinName kinda routine if available ? Line 204: return ChangeConfig(context, consensus::REMOVE_SERVER); I don't see you taking care of REMOVE_SERVER anywhere in ChangeConfig. In fact original impl also did not seem to care about it. I am missing something prolly. PS4, Line 303: VOTER This need not have been explicit if there are only two types - i.e, if the parameter did not specify VOTER, it means peer is non-voter. But again, this also means it is not flexible to accommodate more 'type' in future, so this is fine. -- To view, visit http://gerrit.cloudera.org:8080/4180 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id6e96fc5b0106946f29a2faee8e2667be738b740 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
