Adar Dembo has posted comments on this change.

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

Patch Set 4:

File src/kudu/tools/

Line 293:   unique_ptr<Action> add_server =
> I missed that it's already sorted in BuildHelp, never mind then.
There's no sorting in Action::BuildHelp() or Mode::BuildHelp(). But aren't the 
actions already sorted alphabetically? I've ordered them as "delete_table", 
"ksck", and "list_tables"; that's alphabetical, no?
File src/kudu/tools/

Line 91: const char* const kTabletIdArg = "tablet_id";
> nice, i can consolidate in kudu-test-tool as well like this, but why not co
It's a Google style guide convention:

PS4, Line 117: tablet_raw
> Bear with a basic Qn: what prevents us from using unique_ptr directly inste
I'm not aware of a way to "offer" the internal pointer stored by the unique_ptr 
such that it can be passed to GetTablet() (which only accepts a pointer to a 
pointer as an argument).

PS4, Line 177: kudu
> some const string again ? or GetBinName kinda routine if available ?
This is just a random sequence of characters; its value doesn't matter, and 
since I'm not reusing it elsewhere, I've just inlined the value.

Line 204:   return ChangeConfig(context, consensus::REMOVE_SERVER);
> I don't see you taking care of REMOVE_SERVER anywhere in ChangeConfig. In f
Check out L186: the cc_type is stored directly in the ChangeConfigRequestPB 
class. REMOVE_SERVER is unique in that it doesn't need anything on top of that.

PS4, Line 303: VOTER
> This need not have been explicit if there are only two types - i.e, if the 
Yeah, that's why I've set it up this way.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e96fc5b0106946f29a2faee8e2667be738b740
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to