[kudu-CR] tool: better handling for positional arguments

2016-08-19 Thread Todd Lipcon (Code Review)
Todd Lipcon has submitted this change and it was merged.

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


tool: better handling for positional arguments

This patch modifies Action to describe all of its argument requirements, not
just gflags. As a result, we can improve usability in two major ways:
1. The parser itself can process positional arguments, leaving less work for
   individual actions.
2. The action help strings can describe positional arguments.

Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Reviewed-on: http://gerrit.cloudera.org:8080/4013
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/subprocess.cc
6 files changed, 246 insertions(+), 134 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: better handling for positional arguments

2016-08-19 Thread Todd Lipcon (Code Review)
Todd Lipcon has posted comments on this change.

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


Patch Set 4: Code-Review+2

-- 
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: better handling for positional arguments

2016-08-18 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change.

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


Patch Set 3:

(1 comment)

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

Line 145:const boost::optional& 
variadic_args) const {
> why an optional vector instead of just an empty vector inthe case that ther
Done


-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: better handling for positional arguments

2016-08-18 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

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


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/2985/

-- 
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: better handling for positional arguments

2016-08-18 Thread Adar Dembo (Code Review)
Hello Todd Lipcon, Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4013

to look at the new patch set (#4).

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

tool: better handling for positional arguments

This patch modifies Action to describe all of its argument requirements, not
just gflags. As a result, we can improve usability in two major ways:
1. The parser itself can process positional arguments, leaving less work for
   individual actions.
2. The action help strings can describe positional arguments.

Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
---
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/subprocess.cc
6 files changed, 246 insertions(+), 134 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/4013/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4013
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: better handling for positional arguments

2016-08-18 Thread Todd Lipcon (Code Review)
Todd Lipcon has posted comments on this change.

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


Patch Set 3:

(1 comment)

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

Line 145:const boost::optional& 
variadic_args) const {
why an optional vector instead of just an empty vector inthe case that there 
are none? shouldn't the tool already know on its own whether it wanted them?


-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: better handling for positional arguments

2016-08-18 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

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


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/2981/

-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: better handling for positional arguments

2016-08-18 Thread Adar Dembo (Code Review)
Hello Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4013

to look at the new patch set (#3).

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

tool: better handling for positional arguments

This patch modifies Action to describe all of its argument requirements, not
just gflags. As a result, we can improve usability in two major ways:
1. The parser itself can process positional arguments, leaving less work for
   individual actions.
2. The action help strings can describe positional arguments.

Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
---
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/subprocess.cc
6 files changed, 247 insertions(+), 134 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/4013/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4013
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: better handling for positional arguments

2016-08-18 Thread Todd Lipcon (Code Review)
Todd Lipcon has posted comments on this change.

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


Patch Set 1:

(1 comment)

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

PS1, Line 187:  All remaining arguments on the command line will be joined (via 
single
 :   // space delimiter) to construct the parameter value.
> I had three options:
yea, option #2 seems more natural to me (most libraries I've used present the 
leftover flags as 'argv')


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: better handling for positional arguments

2016-08-18 Thread Adar Dembo (Code Review)
Hello Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4013

to look at the new patch set (#2).

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

tool: better handling for positional arguments

This patch modifies Action to describe all of its argument requirements, not
just gflags. As a result, we can improve usability in two major ways:
1. The parser itself can process positional arguments, leaving less work for
   individual actions.
2. The action help strings can describe positional arguments.

Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
---
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/subprocess.cc
6 files changed, 246 insertions(+), 132 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/4013/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4013
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] tool: better handling for positional arguments

2016-08-18 Thread Kudu Jenkins (Code Review)
Kudu Jenkins has posted comments on this change.

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


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/2974/

-- 
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: better handling for positional arguments

2016-08-17 Thread Dan Burkert (Code Review)
Dan Burkert has posted comments on this change.

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


Patch Set 1:

(2 comments)

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

PS1, Line 135: marshaled
I think it's clearer to just say 'put into the map'.


PS1, Line 191: Required
Why are variadic params required?


-- 
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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: better handling for positional arguments

2016-08-17 Thread Todd Lipcon (Code Review)
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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes


[kudu-CR] tool: better handling for positional arguments

2016-08-17 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change.

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


Patch Set 1:

> (5 comments)
 > 
 > There is a Go library which allows to implement rich CLI tools.  If
 > using the library it's really easy to document the
 > commands/sub-commands/parameters and it provides tab-completion
 > support for shell as an additional feature.  It might be useful to
 > take a look at that:
 > https://www.progville.com/go/cli-go-better-command-line-applications-go/
 > https://github.com/urfave/cli
 > 
 > There is even more powerful library named Cobra:
 > https://github.com/spf13/cobra
 > 
 > We implemented very decent command-line tool using codegangsta/cli
 > library in one of prior projects.
 > 
 > Also, if talking about command-line tools: given the abundance of
 > CLI libraries, it might make sense to consider an option to
 > implement those in Go.  It's really fast and simple in development
 > and seems to be the perfect language choice for that sort of
 > things.

I re-read my comment and it seemed like I was too much advocating for using 
Golang :)

In fact, I mostly meant it might be useful/insightful to take a look at the cli 
package.  To me it looked very convenient the way how to specify commands, 
sub-commands and options in the code.  Also, if it makes sense to exercise bash 
completion to in displaying available sub-commands, it would also make sense to 
check that.  Basically, it's about outputting the available sub-commands if 
given --enable-bash-completion option at any level.

-- 
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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No


[kudu-CR] tool: better handling for positional arguments

2016-08-17 Thread Alexey Serbin (Code Review)
Alexey Serbin has posted comments on this change.

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


Patch Set 1: Code-Review+1

(5 comments)

There is a Go library which allows to implement rich CLI tools.  If using the 
library it's really easy to document the commands/sub-commands/parameters and 
it provides tab-completion support for shell as an additional feature.  It 
might be useful to take a look at that:
  https://www.progville.com/go/cli-go-better-command-line-applications-go/
  https://github.com/urfave/cli

There is even more powerful library named Cobra:
  https://github.com/spf13/cobra

We implemented very decent command-line tool using codegangsta/cli library in 
one of prior projects.

Also, if talking about command-line tools: given the abundance of CLI 
libraries, it might make sense to consider an option to implement those in Go.  
It's really fast and simple in development and seems to be the perfect language 
choice for that sort of things.

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

PS1, Line 54: int
Nit: would you mind to use string::size_type instead of int for first_dash_idx 
variable?


PS1, Line 164: int
string::size_type ?


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

Line 21: #include 
Nit: if I'm not mistaken, by the code style the boost and glog headers should 
come after STL headers and before kudu-specific ones.


Line 162:   std::vector optional;
Does it make sense to have description for an optional parameter as well?  As I 
see, both required and variadic ones have descriptions.


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

Line 60:   const ActionArgDescriptor* args = >args();
What are benefits of using a pointer instead of a reference here?  Is it 
because operator-> is overloaded for the operand?  I.e. why not

const ActionArgDescriptor& args = action->args();


-- 
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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes