Alexey Serbin has posted comments on this change. Change subject: Convert pbc-dump over to new tool infrastructure ......................................................................
Patch Set 1: (7 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'? 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 parameter? If yes, does it make sense to have those literals defined at one place? PS1, Line 45: string const string& instead of string? As I see the FindOrDie() utility function returns a reference, so why to copy? PS1, Line 45: "path" Does it make sense to declare a constant for the "path" option? I see it's used in a couple places at least in this file. PS1, Line 62: "oneline" This and 'online' parameter at line 36: do these correspond to the same parameter? If yes, does it make sense to have those literals defined at one place? PS1, Line 63: "path" A constant for the name of the parameter? Also, I see there is a flag definition for the 'oneline' parameter but there isn't one for 'path'. Is it intentional? 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-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-HasComments: Yes
