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 <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
