Todd Lipcon has posted comments on this change.

Change subject: tool: better handling for positional arguments
......................................................................


Patch Set 1:

(4 comments)

Sure wish there was some test coverage on these tools...

http://gerrit.cloudera.org:8080/#/c/4013/1/src/kudu/tools/tool_action.h
File src/kudu/tools/tool_action.h:

PS1, Line 149: ActionArg
the name sounds like it's a descriptor for one argument, whereas really it's a 
descriptor for all of the arguments. Rename to ArgsDescriptor? or 
ActionArgsInfo?


Line 150:   struct Arg {
I think this being a nested struct is a bit confusing, because, best I can 
tell, 'ActionArgDescriptor' is an internal thing, and yet now you've got it 
exposed here looking like it's part of the API.


PS1, Line 187:  All remaining arguments on the command line will be joined (via 
single
             :   // space delimiter) to construct the parameter value.
I find this odd vs an array.. otherwise you couldn't use this to pass a list of 
files, where the files may have spaces in their name... eg: kudu log-dump 
"/home/todd/My Stuff/log-1" "/home/todd/My Stuff/log-2"

Also worth noting that this inherently verifies that at least one such argument 
is passed, right?


Line 196:   // This parameter will be parsed as a gflag, and thus can be 
provided by the
not clear what "parsed as a gflag" means. Is 'param' the _name_ of a gflag? ie 
you should have used normal gflag macros to define it, and here you are just 
referring to it? What happens if you specify an undefined flag? Crash?

Does the value of this flag end up in the 'args' map? Or just set into the 
gflag?


-- 
To view, visit http://gerrit.cloudera.org:8080/4013
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to