Adar Dembo has posted comments on this change.

Change subject: tool: port ts-cli

Patch Set 2:


 > I agree from the user perspective it's nice to expose the generic
 > stuff in both 'master' and 'tserver', but wonder if we could share
 > slightly more code between them. Could you reuse the same 'action'
 > in both places?

I started to implement this, but it got ugly pretty fast:
1. Need an enum to differentiate between the two server types. OK, fine.
2. Need to bind that enum to each function so the function itself knows what 
variant it is (e.g. in order to use the right default port). A little ugly, but 
3. When building the actions, need to use the enum values to figure out what 
argument names to use, and whether to use "Tablet Server" or "Master" in the 
action and argument descriptions. Er...
4. Need to consider each enum value in each function in order to FindOrDie() 
the argument with the right name (unless we change 'master_address' and 
'tserver_address' to just 'address', but I'm trying to keep it nice for the 
user). Gah, not good.

I didn't mind #1, #2, and #3, but #4 (a switch statement per function) made it 
look too ugly.

So unless you can see a cleaner way to do this, I'd rather leave it as-is.
File src/kudu/tools/

Line 53: DECLARE_int64(timeout_ms); // defined in ksck
> maybe move to 'common' since it's a "common" flag?
Doing that would break the encapsulation of ksck in that it (as a 
module/library/whatever) would now depend on the CLI tool.

I wasn't quite sure how to handle it, so I went with this quick and dirty hack.

Line 55: DEFINE_bool(force, false, "If true, allows the set_flag command to set 
a flag "
> I foresee a future issue here: I bet we'll have other flags with '--force' 
Yeah. It may make sense to stop using gflags for optional parameters at that 
juncture. We'll see.

Line 213:   MessengerBuilder builder("kudu");
> odd name
Eh, the name doesn't matter. But I'll use 'tool' instead.

Line 214:   RETURN_NOT_OK(builder.Build(&messenger));
> perhaps just define the builder inline here? MessengerBuilder("tool").Build
File src/kudu/tools/tool_action_common.h:

PS2, Line 44: typename T
> I think 'class ProxyClass' would be clearer (I usually only use 'typename' 

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb5a59fd690c2dd09e4e76858469d81f9d501371
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to