Todd Lipcon has posted comments on this change.

Change subject: tool: port ts-cli
......................................................................


Patch Set 2:

(5 comments)

I agree from the user perspective it's nice to expose the generic stuff in both 
'master' and 'tserver', but wonder if we could share slightly more code between 
them. Could you reuse the same 'action' in both places?

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

Line 53: DECLARE_int64(timeout_ms); // defined in ksck
maybe move to 'common' since it's a "common" flag?


Line 55: DEFINE_bool(force, false, "If true, allows the set_flag command to set 
a flag "
I foresee a future issue here: I bet we'll have other flags with '--force' 
arguments, and we're going to need to have per-subtool overrides of this help 
text. No action here yet but just noticing that our yak is looking a little 
hairy and may need some shaving soon.


Line 213:   MessengerBuilder builder("kudu");
odd name


Line 214:   RETURN_NOT_OK(builder.Build(&messenger));
perhaps just define the builder inline here? 
MessengerBuilder("tool").Build(&messenger)


http://gerrit.cloudera.org:8080/#/c/4373/2/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:

PS2, Line 44: typename T
I think 'class ProxyClass' would be clearer (I usually only use 'typename' if 
it is expected to plausibly be a primitive)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb5a59fd690c2dd09e4e76858469d81f9d501371
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to