Adar Dembo 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 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?

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 i

Line 114:   const char* kTableName = "test";
> string

Line 132:       args.push_back(kBinPath);
> I think using a brace initializer list here is nicer

Line 164:       args.push_back(Substitute("$0:$1", m.first, 
> 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 

  opid_index: -1
  peers {
    permanent_uuid: "ace6f4bf66404dd9a48a6347d92136c8"
    member_type: VOTER
    last_known_addr {
      host: ""
      port: 11010
  peers {
    permanent_uuid: "0a4afd4e53f248dc9f2c8ccc65096fdc"
    member_type: VOTER
    last_known_addr {
      host: ""
      port: 11011
  peers {
    permanent_uuid: "1c7f19e7ecad4f918c0d3d23180fdb18"
    member_type: VOTER
    last_known_addr {
      host: ""
      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.
File src/kudu/master/

PS4, Line 117: remote_bootstrap_service
> should be updated to the new name, right?
File src/kudu/master/sys_catalog.h:

PS4, Line 67: static constexpr const char*
> static const string
No love for static char pointers? Okay.
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:
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.
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

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

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++ 
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
Sure. I've also split this up into two methods. Didn't really add much by being 

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

PS4, Line 107: .pre_rewrite
> maybe add a timestamp?

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: = &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'? 

Line 175:   tablet.sub_actions = {
> same here - a builder type thing which automatically sets &BuildNonLeafActi
See above.
File src/kudu/tools/

Line 40: extern Action BuildFsAction();
> why not put these as normal prototypes in tool_action.h? odd to see them he

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

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: 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