Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8764 )
Change subject: KUDU-1097: replacement replica after cluster restart ...................................................................... Patch Set 1: (10 comments) This is a good test to have. http://gerrit.cloudera.org:8080/#/c/8764/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8764/1//COMMIT_MSG@7 PS1, Line 7: nit: add test for http://gerrit.cloudera.org:8080/#/c/8764/1//COMMIT_MSG@12 PS1, Line 12: promoted are promoted http://gerrit.cloudera.org:8080/#/c/8764/1/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc: http://gerrit.cloudera.org:8080/#/c/8764/1/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1512 PS1, Line 1512: nit: the http://gerrit.cloudera.org:8080/#/c/8764/1/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1515 PS1, Line 1515: promoted nit: are promoted http://gerrit.cloudera.org:8080/#/c/8764/1/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1524 PS1, Line 1524: WhileReplacing naming nit: how about: WithNonVoter, since replacing is an overloaded term for 3-4-3 with replace=true http://gerrit.cloudera.org:8080/#/c/8764/1/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1535 PS1, Line 1535: // Need some extra tablet servers to ensure the catalog manager does not : // replace almost all the replicas, even if it could. : FLAGS_num_tablet_servers = 2 * kReplicasNum + 1; I wonder if instead of doing this, we could just shut down the master after it adds the new non-voter. http://gerrit.cloudera.org:8080/#/c/8764/1/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1607 PS1, Line 1607: const auto it = tablet_servers_.find(new_replica_uuid); : ASSERT_NE(tablet_servers_.end(), it); : TServerDetails* ts_new_replica = it->second; nit: can replace this with: TServerDetails* ts_new_replica = FindOrDie(tablet_servers_, new_replica_uuid); http://gerrit.cloudera.org:8080/#/c/8764/1/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1618 PS1, Line 1618: e.first == tablet_id_ nit: perhaps make this a DCHECK or even assert on it outside the loop? This test only creates one tablet. http://gerrit.cloudera.org:8080/#/c/8764/1/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1655 PS1, Line 1655: //NO_FATALS(WaitForReplicasAndUpdateLocations(table_->name())); ? http://gerrit.cloudera.org:8080/#/c/8764/1/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1670 PS1, Line 1670: EXPECT_TRUE(IsRaftConfigVoter(new_replica_uuid, cstate.committed_config())) I'm a bit concerned about all those extra replicas and stuff. Is this test reliable? See my comment above about the extra replicas. -- To view, visit http://gerrit.cloudera.org:8080/8764 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia3760d0e454c9215c0f88bd8f1bed68999935a1c Gerrit-Change-Number: 8764 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Comment-Date: Sat, 09 Dec 2017 02:17:37 +0000 Gerrit-HasComments: Yes
