Adar Dembo has posted comments on this change. Change subject: Add 'kudu tserver list' tool ......................................................................
Patch Set 5: (7 comments) In case you missed it, I responded to your "how much testing" question earlier with some suggestions. 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. 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. 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")) { > I think it's easier than having two separate timeout options. Agreed, but why do we need to override the RPC timeout at all? 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? 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: > That throws away the master error information. Be that as it may, it's more consistent with how we build statuses from errors elsewhere in the CLI. How about doing what CopyReplica does as a middle ground: RETURN_NOT_OK(proxy->StartTabletCopy(req, &resp, &rpc)); if (resp.has_error()) { RETURN_NOT_OK_PREPEND( StatusFromPB(resp.error().status()), strings::Substitute("Remote server returned error code $0", TabletServerErrorPB::Code_Name(resp.error().code()))); } Line 189 > I originally added it to the cluster mode, but I think it's more discoverab Can you update the Description then to something like "Operate on Kudu Tablet Servers"? 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, ",")) { > warning: the variable '__end' is copy-constructed from a const reference bu Do you want strings::SkipEmpty() here? Then you could omitthe handling of column.empty(). -- 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 <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
