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 <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

Reply via email to