Todd Lipcon has posted comments on this change.

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

Patch Set 1:

Commit Message:

Line 7: Convert pbc-dump over to new tool infrastructure
> nit: 'to the new tool infrastructure'?
File src/kudu/tools/CMakeLists.txt:

Line 90
> Note that removing the executable will cause the packaging builds to fail. 
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
File src/kudu/tools/

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.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7707f004ea31d1a9e7bb890611080785f667c78
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to