You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/08/10 05:06:04 UTC
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Hello Mike Percy, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/3880
to review the following change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
KUDU-1474: single to multi-master deployment migration
This patch introduces the machinery needed to migrate from a single-node
master deployment to multi-master:
1. Inclusion of the tablet copy service in the master. This was easy; the
master module already depends on 'tserver', so registering the service is
just a few lines of code.
2. The new "kudu" command line tool, whose operations are used in migration.
It'll also serve as the basis for KUDU-619.
3. An end-to-end integration test to showcase the migration.
A couple notes about the new tool. I began with a gflags-based approach, but
found it unwieldy after a while, because it's tough to provide a good help
experience, and the lack of positional argument support makes for long
command lines.
I briefly looked at boost but didn't want to reintroduce a library dependency,
so I ended up writing my own (simple) parser. It maps command line arguments to
"actions" and allows for arbitrary levels of nesting, so you can do things like
"kudu tablet copy blah" as well as "kudu fs dump". At each level, if
insufficient arguments are provided, an informative help message is printed
though there's room for improvement here.
Change-Id: I89c741381ced3731736228cd07fe85106ae72541
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tools/CMakeLists.txt
A src/kudu/tools/tool_actions.cc
A src/kudu/tools/tool_actions.h
A src/kudu/tools/tool_actions_fs.cc
A src/kudu/tools/tool_actions_fs.h
A src/kudu/tools/tool_actions_tablet.cc
A src/kudu/tools/tool_actions_tablet.h
A src/kudu/tools/tool_main.cc
14 files changed, 944 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/3880/1
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3880
to look at the new patch set (#6).
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
KUDU-1474: single to multi-master deployment migration
This patch introduces the machinery needed to migrate from a single-node
master deployment to multi-master:
1. Inclusion of the tablet copy service in the master. This was easy; the
master module already depends on 'tserver', so registering the service is
just a few lines of code.
2. The new "kudu" command line tool, whose operations are used in migration.
It'll also serve as the basis for KUDU-619.
3. An end-to-end integration test to showcase the migration.
A couple notes about the new tool. I began with a gflags-based approach, but
found it unwieldy after a while, because it's tough to provide a good help
experience, and the lack of positional argument support makes for long
command lines.
I briefly looked at boost but didn't want to reintroduce a library dependency,
so I ended up writing my own (simple) parser. It maps command line arguments to
"actions" and allows for arbitrary levels of nesting, so you can do things like
"kudu tablet copy blah" as well as "kudu fs dump". At each level, if
insufficient arguments are provided, an informative help message is printed
though there's room for improvement here.
Change-Id: I89c741381ced3731736228cd07fe85106ae72541
---
M build-support/dist_test.py
M src/kudu/gutil/strings/join.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tools/CMakeLists.txt
A src/kudu/tools/tool_action.cc
A src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_fs.cc
A src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_main.cc
15 files changed, 927 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/3880/6
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3880
to look at the new patch set (#2).
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
KUDU-1474: single to multi-master deployment migration
This patch introduces the machinery needed to migrate from a single-node
master deployment to multi-master:
1. Inclusion of the tablet copy service in the master. This was easy; the
master module already depends on 'tserver', so registering the service is
just a few lines of code.
2. The new "kudu" command line tool, whose operations are used in migration.
It'll also serve as the basis for KUDU-619.
3. An end-to-end integration test to showcase the migration.
A couple notes about the new tool. I began with a gflags-based approach, but
found it unwieldy after a while, because it's tough to provide a good help
experience, and the lack of positional argument support makes for long
command lines.
I briefly looked at boost but didn't want to reintroduce a library dependency,
so I ended up writing my own (simple) parser. It maps command line arguments to
"actions" and allows for arbitrary levels of nesting, so you can do things like
"kudu tablet copy blah" as well as "kudu fs dump". At each level, if
insufficient arguments are provided, an informative help message is printed
though there's room for improvement here.
Change-Id: I89c741381ced3731736228cd07fe85106ae72541
---
M build-support/dist_test.py
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tools/CMakeLists.txt
A src/kudu/tools/tool_actions.cc
A src/kudu/tools/tool_actions.h
A src/kudu/tools/tool_actions_fs.cc
A src/kudu/tools/tool_actions_fs.h
A src/kudu/tools/tool_actions_tablet.cc
A src/kudu/tools/tool_actions_tablet.h
A src/kudu/tools/tool_main.cc
15 files changed, 945 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/3880/2
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 4:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/integration-tests/master_migration-itest.cc
File src/kudu/integration-tests/master_migration-itest.cc:
Line 44: using std::string;
using std::shared_ptr;
Line 69: cluster_.reset();
This reset is redundant, right?
Line 80: const std::string& table_name) {
indentation
Line 109: return scanner.Open();
This seems kind of fragile (relying on Open to actually talk to a tserver is way outside the purview of this test). I think a more robust (and simpler) way would be to just call ASSERT_EQ(0, CountTableRows())
Line 114: const char* kTableName = "test";
string
Line 164: args.push_back(Substitute("$0:127.0.0.1:$1", m.first, m.second));
question: this is implying that both UUID and address are added to the raft config? Is that true for tablets as well as masters?
PS4, Line 173: it cannot replicate to them
because they aren't up, or because they aren't caught up?
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/master/master.cc
File src/kudu/master/master.cc:
PS4, Line 117: remote_bootstrap_service
should be updated to the new name, right?
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/master/sys_catalog.h
File src/kudu/master/sys_catalog.h:
PS4, Line 67: static constexpr const char*
static const string
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_action.cc
File src/kudu/tools/tool_action.cc:
PS4, Line 53:
extra space?
PS4, Line 58: BuildLeafActionHelpString
Where does it actually print out the arguments required for the leaf action?
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_action.h
File src/kudu/tools/tool_action.h:
PS4, Line 51: char*
Any reason to prefer raw pointers to string here?
Line 69: std::string BuildActionChainString(std::vector<Action> chain);
It seems like at least some, if not all, of these could take the chain by const ref.
PS4, Line 91: std::list
Maybe it would indicate intent more if you used a std::deque here, unless I missed something and you do actually need random removal.
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:
Line 56: Action BuildFsAction() {
I mean this terse syntax is great and all, but I think what we really need is some kind of XML layout and documentation engine/generator to really give our CLI some pop.
Line 57: Action fs_format;
I think you can use struct literal syntax here to clean this up, something like:
Action fs_format = {
"format",
"Format a new Kudu filesystem",
...
}
Although you may prefer the explicitness.
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_action_tablet.cc
File src/kudu/tools/tool_action_tablet.cc:
PS4, Line 107: .pre_rewrite
maybe add a timestamp?
PS4, Line 111: gscoped_ptr
unique_ptr
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
KUDU-1474: single to multi-master deployment migration
This patch introduces the machinery needed to migrate from a single-node
master deployment to multi-master:
1. Inclusion of the tablet copy service in the master. This was easy; the
master module already depends on 'tserver', so registering the service is
just a few lines of code.
2. The new "kudu" command line tool, whose operations are used in migration.
It'll also serve as the basis for KUDU-619.
3. An end-to-end integration test to showcase the migration.
A couple notes about the new tool. I began with a gflags-based approach, but
found it unwieldy after a while, because it's tough to provide a good help
experience, and the lack of positional argument support makes for long
command lines.
I briefly looked at boost but didn't want to reintroduce a library dependency,
so I ended up writing my own (simple) parser. It maps command line arguments to
"actions" and allows for arbitrary levels of nesting, so you can do things like
"kudu tablet copy blah" as well as "kudu fs dump". At each level, if
insufficient arguments are provided, an informative help message is printed
though there's room for improvement here.
Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Reviewed-on: http://gerrit.cloudera.org:8080/3880
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
---
M build-support/dist_test.py
M src/kudu/gutil/strings/join.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tools/CMakeLists.txt
A src/kudu/tools/tool_action.cc
A src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_fs.cc
A src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_main.cc
15 files changed, 926 insertions(+), 11 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 4:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:
Line 259: std::string GetBinaryPath(const std::string& binary) const;
doc these now that they're public
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/integration-tests/master_migration-itest.cc
File src/kudu/integration-tests/master_migration-itest.cc:
Line 132: args.push_back(kBinPath);
I think using a brace initializer list here is nicer
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_action.cc
File src/kudu/tools/tool_action.cc:
PS4, Line 36: for (int i = 0; i < chain.size(); i++) {
: if (i > 0) {
: msg += " ";
: }
: msg += chain[i].name;
: }
maybe it's time for us to write a utility function in stl_util like:
deque<T> MapItems(const COLLECTION& c, FUNC func) {
deque<T> ret;
for (const auto& el : c) {
ret.push_back(F(el));
}
return ret;
}
so that we could implement this function in a nice way like:
return JoinStrings(MapItems(chain, [](const Action& a) { return a.name; }), " ");
(not certain the above impl actually works in terms of template magic, but you get the idea)
Also if you think this is overkill, feel free to ignore
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_action.h
File src/kudu/tools/tool_action.h:
PS4, Line 91: std::list
> Maybe it would indicate intent more if you used a std::deque here, unless I
also it's funny that you pass std::vector<string> into the 'Run' functions, but then you make them convert to list here. Perhaps it woudl be easier on implementers to just give them the list to begin with.
PS4, Line 103: td::move(chain))
agreed that const ref arguments for these things would make more sense, since there is no 'consume' relationship going on.
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_action_tablet.cc
File src/kudu/tools/tool_action_tablet.cc:
Line 53: Status ParsePeerString(const string& peer_str,
could do with a doc explaining what the syntax this is parsing is
Line 78: *peer = { std::move(uuid), std::move(hostport) };
maybe I'm a luddite, but I find these std::move()s hurt readability, and this isn't exactly perf-critical code. Same true elsewhere in the patch.
Line 91: peers.emplace_back(std::move(parsed_peer));
same here, i'd rather just stick with vanilla push_back
Line 126: cmeta->set_committed_config(new_config);
one interesting thing here is that we're changing the raft config without actually bumping its config opid index. That's OK in the case of the use case you're intending here, but it makes it less useful in the context of a non-master tablet, since we need the opid_index bump in order for the master to "notice" the changed config. Perhaps we should add some kind of check that this is only used with the special tablet ID for the master for now? (perhaps with a flag override, or maybe just a big WARNING or something?)
Line 162: tablet_rewrite_raft_config.help = &BuildLeafActionHelpString;
what about a factory method or builder like Action::BuildLeafAction("rewrite_raft_config").set_description(...).set_run(&RewriteRaftConfig)
or somesuch? It seems weird to have to specify the &BuildLeafActionHelpString thing here which is conceptually 'internal'
Line 173: tablet.description = "Operate on a local Kudu tablet";
would it be more correct/clear to call this 'replica' instead of 'tablet'? that makes it more clear that it's just the local replica, and you aren't somehow re-assigning the raft config globally
Line 175: tablet.sub_actions = {
same here - a builder type thing which automatically sets &BuildNonLeafActionHelpString would be nice
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
Line 40: extern Action BuildFsAction();
why not put these as normal prototypes in tool_action.h? odd to see them here.
Line 55: int RunTool(Action root, int argc, char** argv) {
this stuff is complex enough that it would be nice to have a unit test for the 'kudu' tool itself (eg checking --help, default output, etc)
Line 96: sub_actions = cur_action->sub_actions;
this line is unnecessary, right? done on line 61
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/2783/
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 7:
Build Started http://104.196.14.100/job/kudu-gerrit/2931/
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/master/sys_catalog.h
File src/kudu/master/sys_catalog.h:
PS4, Line 67: static constexpr const char*
> No love for static char pointers? Okay.
Doesn't actually work; cpplint complains about using std::string for global string constants; it wants us to use const char* instead.
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 6:
Build Started http://104.196.14.100/job/kudu-gerrit/2880/
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/3880/6/src/kudu/gutil/strings/join.h
File src/kudu/gutil/strings/join.h:
Line 204: string JoinStrings(const CONTAINER& components,
> some comment doc here would be good. Perhaps a different name as well, like
Done
http://gerrit.cloudera.org:8080/#/c/3880/6/src/kudu/tools/tool_action.cc
File src/kudu/tools/tool_action.cc:
Line 45: msg += "Action can be one of the following:\n";
> this function only seems useful/sensical if 'sub_actions' is non-empty, but
Yeah, L63 is dead code; there's no invocation of BuildNonLeafActionHelpString() with an empty chain. Removed it.
http://gerrit.cloudera.org:8080/#/c/3880/6/src/kudu/tools/tool_action.h
File src/kudu/tools/tool_action.h:
Line 98: Status CheckNoMoreArgs(const std::vector<Action>& chain, CONTAINER args) {
> I think this can take the CONTAINER by const ref.
Done
http://gerrit.cloudera.org:8080/#/c/3880/6/src/kudu/tools/tool_action_tablet.cc
File src/kudu/tools/tool_action_tablet.cc:
Line 124: RETURN_NOT_OK(env_util::CopyFile(env, cmeta_filename, backup_filename, opts));
> worth a LOG(INFO) about 'backing up current config to <filename>'
Done
http://gerrit.cloudera.org:8080/#/c/3880/6/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
Line 69: vector<Action> sub_actions = cur->sub_actions;
> is this mutated? or can it be const auto&?
Done
Line 130: FLAGS_helpfull ||
> it seems like it would be useful to still allow --helpfull or something to
Alright. I'm not sure how useful the real help is, but I won't block it.
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3880
to look at the new patch set (#4).
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
KUDU-1474: single to multi-master deployment migration
This patch introduces the machinery needed to migrate from a single-node
master deployment to multi-master:
1. Inclusion of the tablet copy service in the master. This was easy; the
master module already depends on 'tserver', so registering the service is
just a few lines of code.
2. The new "kudu" command line tool, whose operations are used in migration.
It'll also serve as the basis for KUDU-619.
3. An end-to-end integration test to showcase the migration.
A couple notes about the new tool. I began with a gflags-based approach, but
found it unwieldy after a while, because it's tough to provide a good help
experience, and the lack of positional argument support makes for long
command lines.
I briefly looked at boost but didn't want to reintroduce a library dependency,
so I ended up writing my own (simple) parser. It maps command line arguments to
"actions" and allows for arbitrary levels of nesting, so you can do things like
"kudu tablet copy blah" as well as "kudu fs dump". At each level, if
insufficient arguments are provided, an informative help message is printed
though there's room for improvement here.
Change-Id: I89c741381ced3731736228cd07fe85106ae72541
---
M build-support/dist_test.py
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tools/CMakeLists.txt
A src/kudu/tools/tool_action.cc
A src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_fs.cc
A src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_main.cc
13 files changed, 850 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/3880/4
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 4:
Build Started http://104.196.14.100/job/kudu-gerrit/2810/
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/integration-tests/master_migration-itest.cc
File src/kudu/integration-tests/master_migration-itest.cc:
Line 44: using kudu::master::SysCatalogTable;
> Doesn't work; I'm already using client::sp::shared_ptr, and I need to use t
ah, I misunderstood, I thought it was just missing.
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/master/sys_catalog.h
File src/kudu/master/sys_catalog.h:
PS4, Line 67: static const char* const kSy
> Doesn't actually work; cpplint complains about using std::string for global
Ah ok, I didn't know about the potential for a shutdown race condition.
http://gerrit.cloudera.org:8080/#/c/3880/6/src/kudu/tools/tool_action.h
File src/kudu/tools/tool_action.h:
Line 98: Status CheckNoMoreArgs(const std::vector<Action>& chain, CONTAINER args) {
I think this can take the CONTAINER by const ref.
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3880
to look at the new patch set (#5).
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
KUDU-1474: single to multi-master deployment migration
This patch introduces the machinery needed to migrate from a single-node
master deployment to multi-master:
1. Inclusion of the tablet copy service in the master. This was easy; the
master module already depends on 'tserver', so registering the service is
just a few lines of code.
2. The new "kudu" command line tool, whose operations are used in migration.
It'll also serve as the basis for KUDU-619.
3. An end-to-end integration test to showcase the migration.
A couple notes about the new tool. I began with a gflags-based approach, but
found it unwieldy after a while, because it's tough to provide a good help
experience, and the lack of positional argument support makes for long
command lines.
I briefly looked at boost but didn't want to reintroduce a library dependency,
so I ended up writing my own (simple) parser. It maps command line arguments to
"actions" and allows for arbitrary levels of nesting, so you can do things like
"kudu tablet copy blah" as well as "kudu fs dump". At each level, if
insufficient arguments are provided, an informative help message is printed
though there's room for improvement here.
Change-Id: I89c741381ced3731736228cd07fe85106ae72541
---
M build-support/dist_test.py
M src/kudu/gutil/strings/join.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tools/CMakeLists.txt
A src/kudu/tools/tool_action.cc
A src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_fs.cc
A src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_main.cc
15 files changed, 923 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/3880/5
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/2795/
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/3880/6/src/kudu/gutil/strings/join.h
File src/kudu/gutil/strings/join.h:
Line 204: string JoinStrings(const CONTAINER& components,
some comment doc here would be good. Perhaps a different name as well, like 'JoinMapped' or 'MapJoinStrings' or somesuch?
http://gerrit.cloudera.org:8080/#/c/3880/6/src/kudu/tools/tool_action.cc
File src/kudu/tools/tool_action.cc:
Line 45: msg += "Action can be one of the following:\n";
this function only seems useful/sensical if 'sub_actions' is non-empty, but on line 63 it gets called with empty sub_actions
http://gerrit.cloudera.org:8080/#/c/3880/6/src/kudu/tools/tool_action_tablet.cc
File src/kudu/tools/tool_action_tablet.cc:
Line 124: RETURN_NOT_OK(env_util::CopyFile(env, cmeta_filename, backup_filename, opts));
worth a LOG(INFO) about 'backing up current config to <filename>'
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
Line 55: return 0;
> While I agree in principle, won't such a test have to be updated with every
yea, though the test could use 'STR_CONTAINS' assertions just to look for whatever first subtool we have. just noticed in the past that these sorts of tools are used infrequently enough that they tend to have breakages we only notice much later.
The fact that there's coverage for the "happy path" is nice, but some basic error-path coverage even if the assertions are loose would be good.
http://gerrit.cloudera.org:8080/#/c/3880/6/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
Line 69: vector<Action> sub_actions = cur->sub_actions;
is this mutated? or can it be const auto&?
Line 130: FLAGS_helpfull ||
it seems like it would be useful to still allow --helpfull or something to get at the real help
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3880
to look at the new patch set (#3).
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
KUDU-1474: single to multi-master deployment migration
This patch introduces the machinery needed to migrate from a single-node
master deployment to multi-master:
1. Inclusion of the tablet copy service in the master. This was easy; the
master module already depends on 'tserver', so registering the service is
just a few lines of code.
2. The new "kudu" command line tool, whose operations are used in migration.
It'll also serve as the basis for KUDU-619.
3. An end-to-end integration test to showcase the migration.
A couple notes about the new tool. I began with a gflags-based approach, but
found it unwieldy after a while, because it's tough to provide a good help
experience, and the lack of positional argument support makes for long
command lines.
I briefly looked at boost but didn't want to reintroduce a library dependency,
so I ended up writing my own (simple) parser. It maps command line arguments to
"actions" and allows for arbitrary levels of nesting, so you can do things like
"kudu tablet copy blah" as well as "kudu fs dump". At each level, if
insufficient arguments are provided, an informative help message is printed
though there's room for improvement here.
Change-Id: I89c741381ced3731736228cd07fe85106ae72541
---
M build-support/dist_test.py
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tools/CMakeLists.txt
A src/kudu/tools/tool_action.cc
A src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_fs.cc
A src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_main.cc
13 files changed, 838 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/3880/3
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3880
to look at the new patch set (#7).
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
KUDU-1474: single to multi-master deployment migration
This patch introduces the machinery needed to migrate from a single-node
master deployment to multi-master:
1. Inclusion of the tablet copy service in the master. This was easy; the
master module already depends on 'tserver', so registering the service is
just a few lines of code.
2. The new "kudu" command line tool, whose operations are used in migration.
It'll also serve as the basis for KUDU-619.
3. An end-to-end integration test to showcase the migration.
A couple notes about the new tool. I began with a gflags-based approach, but
found it unwieldy after a while, because it's tough to provide a good help
experience, and the lack of positional argument support makes for long
command lines.
I briefly looked at boost but didn't want to reintroduce a library dependency,
so I ended up writing my own (simple) parser. It maps command line arguments to
"actions" and allows for arbitrary levels of nesting, so you can do things like
"kudu tablet copy blah" as well as "kudu fs dump". At each level, if
insufficient arguments are provided, an informative help message is printed
though there's room for improvement here.
Change-Id: I89c741381ced3731736228cd07fe85106ae72541
---
M build-support/dist_test.py
M src/kudu/gutil/strings/join.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tools/CMakeLists.txt
A src/kudu/tools/tool_action.cc
A src/kudu/tools/tool_action.h
A src/kudu/tools/tool_action_fs.cc
A src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_main.cc
15 files changed, 926 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/3880/7
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 5:
Build Started http://104.196.14.100/job/kudu-gerrit/2862/
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 4:
(32 comments)
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:
Line 259: std::string GetBinaryPath(const std::string& binary) const;
> doc these now that they're public
Done
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/integration-tests/master_migration-itest.cc
File src/kudu/integration-tests/master_migration-itest.cc:
Line 44: using std::string;
> using std::shared_ptr;
Doesn't work; I'm already using client::sp::shared_ptr, and I need to use that for certain functions I'm calling.
Line 69: cluster_.reset();
> This reset is redundant, right?
Done
Line 80: const std::string& table_name) {
> indentation
Done
Line 109: return scanner.Open();
> This seems kind of fragile (relying on Open to actually talk to a tserver i
Done
Line 114: const char* kTableName = "test";
> string
Done
Line 132: args.push_back(kBinPath);
> I think using a brace initializer list here is nicer
Done
Line 164: args.push_back(Substitute("$0:127.0.0.1:$1", m.first, m.second));
> question: this is implying that both UUID and address are added to the raft
Yes, the RaftConfigPB for any tablet (normal or master) looks something like this:
{
opid_index: -1
peers {
permanent_uuid: "ace6f4bf66404dd9a48a6347d92136c8"
member_type: VOTER
last_known_addr {
host: "127.0.0.1"
port: 11010
}
}
peers {
permanent_uuid: "0a4afd4e53f248dc9f2c8ccc65096fdc"
member_type: VOTER
last_known_addr {
host: "127.0.0.1"
port: 11011
}
}
peers {
permanent_uuid: "1c7f19e7ecad4f918c0d3d23180fdb18"
member_type: VOTER
last_known_addr {
host: "127.0.0.1"
port: 11012
}
}
}
PS4, Line 173: it cannot replicate to them
> because they aren't up, or because they aren't caught up?
The former. I'll clarify.
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/master/master.cc
File src/kudu/master/master.cc:
PS4, Line 117: remote_bootstrap_service
> should be updated to the new name, right?
Done
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/master/sys_catalog.h
File src/kudu/master/sys_catalog.h:
PS4, Line 67: static constexpr const char*
> static const string
No love for static char pointers? Okay.
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_action.cc
File src/kudu/tools/tool_action.cc:
PS4, Line 36: for (int i = 0; i < chain.size(); i++) {
: if (i > 0) {
: msg += " ";
: }
: msg += chain[i].name;
: }
> maybe it's time for us to write a utility function in stl_util like:
As we discussed, implementing MapItems requires template overkill.
But a variant of JoinStrings() that takes a functor isn't too bad. I did that.
PS4, Line 53:
> extra space?
Intentional; I think it improves the readability a bit.
I did remove single quotes though; I found them to be noisy.
PS4, Line 58: BuildLeafActionHelpString
> Where does it actually print out the arguments required for the leaf action
That's still a TODO. :) Currently leaf action positional arguments are parsed in the various run() functions; we'd need to pull them out into some sort of structured description that this can act on.
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_action.h
File src/kudu/tools/tool_action.h:
PS4, Line 51: char*
> Any reason to prefer raw pointers to string here?
Simplicity, but I agree it's not a big deal. Will use strings instead.
Line 69: std::string BuildActionChainString(std::vector<Action> chain);
> It seems like at least some, if not all, of these could take the chain by c
Done
PS4, Line 91: std::list
> also it's funny that you pass std::vector<string> into the 'Run' functions,
Will use a deque instead.
PS4, Line 103: td::move(chain))
> agreed that const ref arguments for these things would make more sense, sin
Done
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:
Line 56: Action BuildFsAction() {
> I mean this terse syntax is great and all, but I think what we really need
Patches welcome!
Line 57: Action fs_format;
> I think you can use struct literal syntax here to clean this up, something
Yes, I like seeing that "format" correspond to name, that "Format a new..." corresponds to description, etc.
What I really want is C99 struct initialization syntax, but that's not supported in C++ (http://stackoverflow.com/questions/5790534/static-structure-initialization-with-tags-in-c).
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_action_tablet.cc
File src/kudu/tools/tool_action_tablet.cc:
Line 53: Status ParsePeerString(const string& peer_str,
> could do with a doc explaining what the syntax this is parsing is
Sure. I've also split this up into two methods. Didn't really add much by being combined.
Line 78: *peer = { std::move(uuid), std::move(hostport) };
> maybe I'm a luddite, but I find these std::move()s hurt readability, and th
But I wanted to be a cool C++11 programmer like Dan. Removed.
Line 91: peers.emplace_back(std::move(parsed_peer));
> same here, i'd rather just stick with vanilla push_back
Done
PS4, Line 107: .pre_rewrite
> maybe add a timestamp?
Done
PS4, Line 111: gscoped_ptr
> unique_ptr
It's required by the caller, but I'll add a patch to replace it with unique_ptr.
Line 126: cmeta->set_committed_config(new_config);
> one interesting thing here is that we're changing the raft config without a
I'll add a warning.
Line 162: tablet_rewrite_raft_config.help = &BuildLeafActionHelpString;
> what about a factory method or builder like Action::BuildLeafAction("rewrit
I'm going to punt on this for now. I've been thinking of how to improve the structure of the tool, and I think separating non-leaf nodes ("modes") from leaf nodes ("actions") makes more sense. I'll do that reorg and add builders while I'm at it.
Line 173: tablet.description = "Operate on a local Kudu tablet";
> would it be more correct/clear to call this 'replica' instead of 'tablet'?
Done
Line 175: tablet.sub_actions = {
> same here - a builder type thing which automatically sets &BuildNonLeafActi
See above.
http://gerrit.cloudera.org:8080/#/c/3880/4/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
Line 40: extern Action BuildFsAction();
> why not put these as normal prototypes in tool_action.h? odd to see them he
Done
Line 55: int RunTool(Action root, int argc, char** argv) {
> this stuff is complex enough that it would be nice to have a unit test for
While I agree in principle, won't such a test have to be updated with every new action, or at least with every top-level action? Not to mention duplication of help strings (or at least substrings) between the test and the tool.
Line 96: sub_actions = cur_action->sub_actions;
> this line is unnecessary, right? done on line 61
Done
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: KUDU-1474: single to multi-master deployment migration
......................................................................
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/2808/
--
To view, visit http://gerrit.cloudera.org:8080/3880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No