Adar Dembo has posted comments on this change. Change subject: Add 'ksck tserver list' tool ......................................................................
Patch Set 4: (17 comments) > @adar - how much testing would you like to see for this commit? Good question. I think we want at least some coverage of the printing methods. Let's not worry about testing exact output (unless you really want to), but for the machine readable ones, can we machine read them and make sure they parse? As for the new action, let's get some coverage in kudu-tool-test for it (i.e. you bring up an external cluster with n tservers and you see n tservers when you run the tool). http://gerrit.cloudera.org:8080/#/c/6654/4/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: Line 372: // Pretty print a table. Maybe a snippet of sample output? Would be nice for the other variants too. PS4, Line 381: Nit: no separation. Line 416: void JsonPrintTable(const vector<string>& headers, const vector<vector<string>>& columns) { Why not use JsonWriter? I think that's way less error prone than rolling your own, and safer to extend in the future too. It'll handle escaping too, maybe? Line 474: .default_rpc_timeout(timeout) Hmm, you sure we want to use timeout_ms as the RPC timeout as well? Line 484: const Req&, Resp*, Nit: indentation. http://gerrit.cloudera.org:8080/#/c/6654/4/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: Line 76: Status SyncLeaderMasterRpc(); Unused and undefined? Line 118: // Initialize the leader master proxy given the provided tool context. Should explain what arguments (required/optional) are referenced. http://gerrit.cloudera.org:8080/#/c/6654/4/src/kudu/tools/tool_action_tserver.cc File src/kudu/tools/tool_action_tserver.cc: Line 41: Nit: since DECLARE_foo are macros, I think they should precede using statements (and thus be contiguous w.r.t. #include statements). PS4, Line 44: DECLARE_bool(show_uuid); : DECLARE_bool(show_rpc_addresses); : DECLARE_bool(show_http_addressses); Where are these used? Line 96: return Status::IOError(SecureShortDebugString(resp.error())); We typically extract the Status via StatusFromPB(resp.error().status()). Line 102: const auto servers = resp.servers(); Not cref? PS4, Line 104: I think our convention is not to separate the capture from the arguments. $ git grep '\[\] (' src | wc -l 14 $ git grep '\[\](' src | wc -l 50 PS4, Line 114: push_back Could be emplace_back? PS4, Line 124: } else if (boost::iequals(column, "rpc-addresses") || : boost::iequals(column, "rpc_addresses")) { Why? I don't think the "underscores are the same as dashes" expectation extends to gflag _values_, just to _keys_. Do we already do this sort of thing elsewhere? Line 175: unique_ptr<Action> list_tservers = Nit: indentation isn't quite right w.r.t. the other actions. Line 178: .ExtraDescription("By default, tablet server UUIDs and RPC addresses will be shown. " So is the idea that different actions that use --columns may have different default values for it? I think there's some function you can call to programatically change a gflag's default value. If you used that, you wouldn't need any of ExtraDescription() because it'd be obvious from the inclusion of --columns' help. I don't know whether it would be easy to program the default value in the context of both running the action and running the tool with --help; could you look into it and share what you find? Line 189: .Description("Operate on a Kudu Tablet Server") The new action doesn't match this; it doesn't operate on a single tablet server. Perhaps it'd be better suited for the 'cluster' mode? -- 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: 4 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-HasComments: Yes
