[kudu-CR] tool: port kudu-admin to 'kudu cluster'

["Dinesh Bhat (Code Review)" <ge...@cloudera.org>]

Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-admin to 'kudu cluster'
......................................................................


Patch Set 3: Code-Review+1

(6 comments)

Few nits, otherwise LGTM.

http://gerrit.cloudera.org:8080/#/c/4180/3/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

PS3, Line 57: kAdminToolName
As I can see we are omitting the word 'admin' altogether, so we can remove those strings from code as well and keep it as 'kuducluster' or something else. Plus renaming the file to kudu-clsuter-test.cc ?


PS3, Line 184: TestTable
We seem to have a const under the tserver namespace from tablet_server-test-base.h : 
const char* TabletServerTestBase::kTableId = "TestTable";


PS3, Line 204: nullptr
I wonder if ASSERT_OK grab the stderr output from Subprocess:Call ? Here we prolly want to know why command failed perhaps ?


PS3, Line 209: TestTable
same as above


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> delete_table =
nit: we could alphabetically order these actions, so when help string is emitted, they are ordered automatically, or else we could sort them out in the BuildHelp routine.


Line 374:   unique_ptr<Mode> change_config =
nit: we can perhaps keep this submode creation under different function called BuildModeChangeConfig and add all actions in that routine to keep the mode/submode actions different from each other. feel free to ignore this since it's only a formatting suggestion.


-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes