Will Berkeley has posted comments on this change. Change subject: [tools] Add a 'kudu tablet relocate' tool ......................................................................
Patch Set 6: (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 Got it. I think I had "pre-voter" and "tombstoned tablets vote" mixed up. 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. Done PS6, Line 458: AnsiCode::YELLOW, "WARNING: " > Should be RED / ERROR? Done PS6, Line 462: RED > YELLOW Done 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: // 5. Profit! > +1 :) PS6, Line 190: iter++ > nit: pre-increment (++iter) is traditionally a more efficient operation to Done Line 247: > nit: extra line Done 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: rem > how about "from_ts_uuid" Done PS6, Line 81: add > how about "to_ts_uuid" Done PS6, Line 142: = Status::OK() > nit: the default constructor of Status gives you Status::OK() Done Line 149: return s.CloneAndPrepend("timed out with ksck errors remaining: last error"); > Actually, we're trying not to do that in new code since we're chaining stat Thanks Adar. PS6, Line 158: c > nit: capitalize See Adar's comment. PS6, Line 162: m > nit: capitalize See Adar's comment. Line 206: Status ChangeConfig(const RunnerContext& context, ChangeConfigType cc_type) { > nit: This and the other functions in this file can all be declared "static" Done Line 378: // TODO(wdb): With pre-voters, it'd be safe to remove right after successfully adding. > see my comment on the commit message Removed. Line 392: leader_uuid, leader_hp, > Nit: indentation. Done PS6, Line 394: f > nit: capitalize See Adar's comment. PS6, Line 443: relocate > "relocate" seems like a fancy synonym for "move". Can we just call this mov Done PS6, Line 448: UUID of the replica to be removed > UUID of the tablet server to move the replica from Done PS6, Line 449: UUID of the replica to be added > UUID of the tablet server to move the replica to Done -- 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: 6 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
