Dan Burkert has posted comments on this change.

Change subject: Add 'kudu tserver list' tool
......................................................................


Patch Set 5:

(7 comments)

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

Line 164:   struct Flag {
> Document.
Done


PS5, Line 173:   // Key-value command line arguments. These must actually 
implemented as
             :   // gflags, which means all that must be specified here are the 
gflag names.
             :   // The gflag definitions themselves will be accessed to get 
the argument
             :   // descriptions.
> Update.
Done


http://gerrit.cloudera.org:8080/#/c/6654/4/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:

Line 474:   } else if (boost::iequals(FLAGS_format, "space")) {
> Agreed, but why do we need to override the RPC timeout at all?
I figured it would be nice to have some control over this when writing scripts, 
so you don't always have to wait for the full timeout before it will fail due 
to a wrong master address or similar.


http://gerrit.cloudera.org:8080/#/c/6654/5/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:

Line 426: // Print a table using JSON formatting.
> Snippet? Or some description of the schema?
Done


http://gerrit.cloudera.org:8080/#/c/6654/4/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:

Line 96: 
> Be that as it may, it's more consistent with how we build statuses from err
Done


Line 189
> Can you update the Description then to something like "Operate on Kudu Tabl
Technically this new functionality doesn't even operate on tablet servers - 
it's just contacting the master.


http://gerrit.cloudera.org:8080/#/c/6654/5/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:

Line 101:   for (const auto& column : strings::Split(FLAGS_columns, ",")) {
> Do you want strings::SkipEmpty() here? Then you could omitthe handling of c
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I047a7675c186a02dd5d8b405431ae885159fcfee
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Dan Burkert <[email protected]>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <[email protected]>
Gerrit-HasComments: Yes

Reply via email to