Todd Lipcon has posted comments on this change.
Change subject: Convert pbc-dump over to new tool infrastructure
Patch Set 1:
Line 7: Convert pbc-dump over to new tool infrastructure
> nit: 'to the new tool infrastructure'?
> Note that removing the executable will cause the packaging builds to fail.
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
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
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 http://gerrit.cloudera.org:8080/4037
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
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>