You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/08/18 06:41:49 UTC
[kudu-CR] Convert pbc-dump over to new tool infrastructure
Hello Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4037
to review the following change.
Change subject: Convert pbc-dump over to new tool infrastructure
......................................................................
Convert pbc-dump over to new tool infrastructure
Change-Id: Ia7707f004ea31d1a9e7bb890611080785f667c78
---
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/tool_action.h
R src/kudu/tools/tool_action_pbc.cc
M src/kudu/tools/tool_main.cc
4 files changed, 33 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/4037/1
--
To view, visit http://gerrit.cloudera.org:8080/4037
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia7707f004ea31d1a9e7bb890611080785f667c78
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] Convert pbc-dump over to the new tool infrastructure
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: Convert pbc-dump over to the new tool infrastructure
......................................................................
Convert pbc-dump over to the new tool infrastructure
Change-Id: Ia7707f004ea31d1a9e7bb890611080785f667c78
Reviewed-on: http://gerrit.cloudera.org:8080/4037
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/pbc-dump.cc
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_pbc.cc
M src/kudu/tools/tool_main.cc
5 files changed, 78 insertions(+), 79 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Alexey Serbin: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4037
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia7707f004ea31d1a9e7bb890611080785f667c78
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Convert pbc-dump over to new tool infrastructure
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Convert pbc-dump over to new tool infrastructure
......................................................................
Patch Set 1:
(3 comments)
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. If you're interested in preventing that, you should first modify install_kudu.sh (CDH/cdh-package.git:kudu) to stop copying kudu-pbc-dump.
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.
I was thinking that modes would correspond directly to contexts. Which means, kudu-pbc-dump would be lumped under a "file" context along with all other operations that operate on a single file by name (e.g. log-dump when called on a segment directly).
Alternatively, we could recognize that "fs" and "file" are similar and reuse "fs".
What do you think?
Line 254: std::unique_ptr<Mode> BuildPbcMode();
Nit: The comments for each of these aren't going to be useful. Let's just combine them like so:
// Returns new nodes for each major mode.
std::unique_ptr<Mode> BuildFooMode();
std::unique_ptr<Mode> BuildBarMode();
...
And let's also keep the modes sorted in alphabetical order.
--
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] Convert pbc-dump over to the new tool infrastructure
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Convert pbc-dump over to the new tool infrastructure
......................................................................
Patch Set 2: Code-Review+1
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Convert pbc-dump over to the new tool infrastructure
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4037
to look at the new patch set (#2).
Change subject: Convert pbc-dump over to the new tool infrastructure
......................................................................
Convert pbc-dump over to the new tool infrastructure
Change-Id: Ia7707f004ea31d1a9e7bb890611080785f667c78
---
M src/kudu/tools/CMakeLists.txt
D src/kudu/tools/pbc-dump.cc
M src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_pbc.cc
M src/kudu/tools/tool_main.cc
5 files changed, 78 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/4037/2
--
To view, visit http://gerrit.cloudera.org:8080/4037
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia7707f004ea31d1a9e7bb890611080785f667c78
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Convert pbc-dump over to the new tool infrastructure
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Convert pbc-dump over to the new tool infrastructure
......................................................................
Patch Set 2:
> Please update install_kudu.sh (on the internal CDH/cdh-package.git:kudu repo and branch) before merging.
Already did.
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Convert pbc-dump over to the new tool infrastructure
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Convert pbc-dump over to the new tool infrastructure
......................................................................
Patch Set 2: Code-Review+2
Please update install_kudu.sh (on the internal CDH/cdh-package.git:kudu repo and branch) before merging.
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Convert pbc-dump over to new tool infrastructure
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Convert pbc-dump over to new tool infrastructure
......................................................................
Patch Set 1:
(1 comment)
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.
> I was thinking that modes would correspond directly to contexts. Which mean
I think my preference is for a not overly-deep hierarchy. The nice thing about a relatively flat hierarchy is that you can just run "kudu -help" and see a good list of all the tools, whereas with a deep hierarchy, you'd have to go hunting through "sub menus" to find the tool you're looking for.
Another way to think about it is that tools should mostly have the form "kudu <noun> <verb>" or in some cases "kudu <noun> <sub-noun> <verb>". Do you disagree?
--
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Convert pbc-dump over to the new tool infrastructure
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Convert pbc-dump over to the new tool infrastructure
......................................................................
Patch Set 1:
(1 comment)
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 63: "path"
> yea, it's a kind of quirk of the tool infrastructure. Optional arguments co
I chose to highlight the "required vs. optional" nature of the parameters, but we can certainly change it to "positional vs. key-value" if you guys think that's more important. I don't have a strong opinion either way.
--
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Convert pbc-dump over to the new tool infrastructure
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Convert pbc-dump over to the new tool infrastructure
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/3029/
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Convert pbc-dump over to new tool infrastructure
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Convert pbc-dump over to new tool infrastructure
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/2976/
--
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Convert pbc-dump over to new tool infrastructure
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] Convert pbc-dump over to new tool infrastructure
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Convert pbc-dump over to new tool infrastructure
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Convert pbc-dump over to new tool infrastructure
......................................................................
Patch Set 1:
(1 comment)
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.
> I think my preference is for a not overly-deep hierarchy. The nice thing ab
Well, "kudu file pbc dump" does fit "kudu <noun> <sub-noun> <verb>". But you're making the case for "kudu pbc dump" which also matches "kudu <noun> <verb>"; presumably you meant that subnouns should be used sparingly.
Anyway, I don't feel strongly, at least not yet. Let's see how it evolves and defer any reorganization until we have more modes and actions.
--
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes