Volodymyr Verovkin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14300 )
Change subject: KUDU-2800 Test long bootstrapping tablet replicas ...................................................................... Patch Set 12: (10 comments) http://gerrit.cloudera.org:8080/#/c/14300/6/src/kudu/integration-tests/raft_config_change-itest.cc File src/kudu/integration-tests/raft_config_change-itest.cc: http://gerrit.cloudera.org:8080/#/c/14300/6/src/kudu/integration-tests/raft_config_change-itest.cc@597 PS6, Line 597: : LOG(INFO) << "Waiting for log GC on " << leader->uuid(); : // Some WAL segments must exist, but wal segment 1 must not exist. : ASSERT_OK(inspect_->WaitForFilePatternInTabletWalDirOnTs( : leader_index, tablet_id_, { "wal-" }, { "wal-000000001" })); : LOG(INFO) << "Log GC complete on " << leader->uuid(); : > I think GetLeaderReplicaWithRetries() handles those cases as well. Or you Done http://gerrit.cloudera.org:8080/#/c/14300/6/src/kudu/integration-tests/raft_config_change-itest.cc@628 PS6, Line 628: : > OK, but given the exact timings, might it be a source for flakiness? Or, p Done http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc File src/kudu/integration-tests/raft_config_change-itest.cc: http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@71 PS11, Line 71: using std::string; > It makes sense to address the issues reported by the Tidy bot. Done http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@465 PS11, Line 465: // Check that 'added_replica_uuid' was added to the consensus > Add short doc for the parameters. Done http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@465 PS11, Line 465: // Check that 'added_replica_uuid' was added to the consensus : // and 'removed_replica_uuid' was removed from consensus. : void ValidateConsensusStateChanged(const string& added_ > nit: align the parameters Done http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@476 PS11, Line 476: ASSERT_FALSE(IsRaftConfigMember(removed_replica_uuid, cstate.committed_config())); > Document the parameters. Done http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@477 PS11, Line 477: > any_replica_server ? Done http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@480 PS11, Line 480: // to quickly fall behind log GC. : // Returns 'any_replica_sever' - UUID of first server with tablet replica, : // and 'no_replica_server' - UUID of server without tablet replica. : void SetUpCluster(bool add_flags_for_log_rolls, : string* any_replica_sever, : string* no_replica_server) : { > nit: document the reasoning behind adding these extra flags and relationshi Done http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@511 PS11, Line 511: > nit: it seems this also can be factored out into a member of the SlowTable Done http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@531 PS11, Line 531: ValidateConsensusStateChanged(any_replica_server, no_replica_server, kTimeout); > nit: add period Done -- To view, visit http://gerrit.cloudera.org:8080/14300 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie1fee42053194f51d7a869ce14788095d6627ed9 Gerrit-Change-Number: 14300 Gerrit-PatchSet: 12 Gerrit-Owner: Volodymyr Verovkin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Comment-Date: Tue, 08 Oct 2019 01:51:57 +0000 Gerrit-HasComments: Yes
