Todd Lipcon has posted comments on this change.

Change subject: KUDU-1474: single to multi-master deployment migration

Patch Set 4:

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
File src/kudu/integration-tests/

Line 132:       args.push_back(kBinPath);
I think using a brace initializer list here is nicer
File src/kudu/tools/

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) {
  return ret;

so that we could implement this function in a nice way like:

return JoinStrings(MapItems(chain, [](const Action& a) { return; }), " 

(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
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.
File src/kudu/tools/

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: = &BuildLeafActionHelpString;
what about a factory method or builder like 

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
File src/kudu/tools/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to