Todd Lipcon has posted comments on this change.

Change subject: Convert pbc-dump over to new tool infrastructure
......................................................................


Patch Set 1:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/4037/1//COMMIT_MSG
Commit Message:

Line 7: Convert pbc-dump over to new tool infrastructure
> nit: 'to the new tool infrastructure'?
Done


http://gerrit.cloudera.org:8080/#/c/4037/1/src/kudu/tools/CMakeLists.txt
File src/kudu/tools/CMakeLists.txt:

Line 90
> Note that removing the executable will cause the packaging builds to fail. 
Done


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

Line 253: // Returns a new "pbc" mode node.
> Well, "kudu file pbc dump" does fit "kudu <noun> <sub-noun> <verb>". But yo
ok, i'll leave as is for now, and once we've migrated everything, we can do a 
quick pass for sanity before release


Line 254: std::unique_ptr<Mode> BuildPbcMode();
> Nit: The comments for each of these aren't going to be useful. Let's just c
Done


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

PS1, Line 36: oneline
> This and "online" literal at line 62: do these correspond to the same param
I don't think the literal can be defined once, since here it's a token and 
below it's a string. Could use preprocessor magic to stringify the token but I 
think it makes it harder to understand vs just duplicating.


PS1, Line 45: "path"
> Does it make sense to declare a constant for the "path" option?  I see it's
added kPathArg constant.

I left the 'string path = ' since it allows a bit more flexibility in the API 
without having to change here, eg the API could decide to pass 'char*' and we 
wouldn't need to change anything. If it were perf critical I think it'd be 
worth it, but here just trying to keep it simple.


PS1, Line 62: "oneline"
> This and 'online' parameter at line 36: do these correspond to the same par
see above


PS1, Line 63: "path"
> A constant for the name of the parameter?
yea, it's a kind of quirk of the tool infrastructure. Optional arguments 
correspond to gflags, and required ones are positional.

Perhaps we should change the API to 'AddPositionalParameter'? Adar?


PS1, Line 66: a
> nit: it seems the 'a' article is not needed here.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7707f004ea31d1a9e7bb890611080785f667c78
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to