Mike Percy has posted comments on this change. Change subject: [tools] Add a 'kudu tablet relocate' tool ......................................................................
Patch Set 4: (20 comments) http://gerrit.cloudera.org:8080/#/c/7444/6//COMMIT_MSG Commit Message: PS6, Line 25: Once pre-voters are implemented, it should be safe to allow the : tool to remove a replica just after adding one, without waiting : for the copy to finish, assuming the tablet stays healthy. I disagree with this statement. The downside to the implementation in this patch is that we can't lose a node while copying else we won't be able to write until the copy is done (because we would have 2/4 running replicas while we would need a strict majority of 3 to write). The window of time that we are in that "fragile" state is the length of time it takes to finish the tablet copy. Adding a node as a pre-voter reduces that window dramatically because we are not changing the number of voters during the tablet copy operation, but we should still wait until the copy is done and the pre-voter has been "promoted" to a voter before removing the "old" voter. If we don't wait, then there will only be 2 voters in the config while the copy takes place which still means that we can't lose a node until the copy is complete (we would need 2/2 to write, but it's even worse than it is right now since we would only have a total of 2 durable replicas during that time instead of 3). http://gerrit.cloudera.org:8080/#/c/7444/6/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: Line 457: ostream& Error() { nit: Would be useful to add a quick one-liner doc on each of these methods. PS6, Line 458: iCode::YELLOW, "WARNING: "); Should be RED / ERROR? PS6, Line 462: , " YELLOW http://gerrit.cloudera.org:8080/#/c/7444/6/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: Line 179: TEST_F(AdminCliTest, TestRelocateTablet) { +1 PS6, Line 190: nit: pre-increment (++iter) is traditionally a more efficient operation to perform on a C++ iterator Line 247: command_args.push_back(peer_uuid); nit: extra line http://gerrit.cloudera.org:8080/#/c/7444/4/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: Line 366: const string& rem_replica_uuid = FindOrDie(context.required_args, kRemReplicaUuidArg); > You are technically correct (the best kind of correct), but the name makes See my comments in rev 6. Basically I agree with Adar here. We should always try to be very precise about our terminology and not introduce new concepts unless we have to. We should stick with TS UUID, the replica is the instance of the tablet on the TS. http://gerrit.cloudera.org:8080/#/c/7444/6/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: PS6, Line 80: add how about "from_ts_uuid" PS6, Line 81: how about "to_ts_uuid" PS6, Line 142: deadline = Mon nit: the default constructor of Status gives you Status::OK() Line 149: } nit: We usually capitalize the first word of a Status message PS6, Line 158: nit: capitalize PS6, Line 162: nit: capitalize Line 206: const string& master_addresses_str = FindOrDie(context.required_args, nit: This and the other functions in this file can all be declared "static", right? Line 378: see my comment on the commit message PS6, Line 394: nit: capitalize PS6, Line 443: "relocate" seems like a fancy synonym for "move". Can we just call this move instead? It's shorter and seems less cryptic. IME, the simpler the verb the better when it comes to administrative tasks. PS6, Line 448: UUID of the tablet server to move the replica from PS6, Line 449: UUID of the tablet server to move the replica to -- To view, visit http://gerrit.cloudera.org:8080/7444 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b7a7243333ba6e6a3d6fce96b220224d6e38a84 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
