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 11: (8 comments) http://gerrit.cloudera.org:8080/#/c/14300/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14300/10//COMMIT_MSG@11 PS10, Line 11: There case are covered: : : 1) Tablet replica bootstraps long time. No data modications happen : during these time. Consensus state does not change. : : 2) Tablet replica is shutdown for some time. During this time a lot of > nit: could you wrap these lines at 72 characters? per #6 of https://chris.b Done http://gerrit.cloudera.org:8080/#/c/14300/10/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/10/src/kudu/integration-tests/raft_config_change-itest.cc@464 PS10, Line 464: ted: > nit: maybe rename this so it's clearer from the callsite what it's trying t Done http://gerrit.cloudera.org:8080/#/c/14300/10/src/kudu/integration-tests/raft_config_change-itest.cc@484 PS10, Line 484: "--consensus_rpc_timeout_ms=2000", : "--follower_unavailable_considered_failed_sec=3" : }; : : if (add_flags_for_log_rolls) { : AddFlagsForLogRolls(&ts_flags); : } : FLAGS_num_tablet_servers = 4; : FLAGS_num_replicas = 3; : NO_FATALS(BuildAndStart(ts_flags, kMasterFlags)); : : ASSERT_EQ(4, tablet_servers_.size()); : : // Extra sanity checks. > nit: is it important for the flags here to be different than the flags in t Done http://gerrit.cloudera.org:8080/#/c/14300/10/src/kudu/integration-tests/raft_config_change-itest.cc@508 PS10, Line 508: as long as there are n > Is this ASSERT_EVENTUALLY necessary? If the replicas have been already move Until restarted replica finishes bootstrap, we may not see desirable state of consensus. http://gerrit.cloudera.org:8080/#/c/14300/10/src/kudu/integration-tests/raft_config_change-itest.cc@516 PS10, Line 516: Shutdown any tablet server with tablet's replica. : auto ts = cluster_->tablet_server_by_uuid(any_replica_server); > Should this check be done on all 3 of the replica_servers? We just want to see that restarted replica has not been evicted and spare server has not been added to consensus. http://gerrit.cloudera.org:8080/#/c/14300/10/src/kudu/integration-tests/raft_config_change-itest.cc@597 PS10, Line 597: > Isn't this just follower_uuid? Done http://gerrit.cloudera.org:8080/#/c/14300/10/src/kudu/integration-tests/raft_config_change-itest.cc@627 PS10, Line 627: : : : : : > How long does it take for this test to finish? Would it make sense to flush We make log rolling frequently via options set in AddFlagsForLogRolls() http://gerrit.cloudera.org:8080/#/c/14300/10/src/kudu/integration-tests/raft_config_change-itest.cc@635 PS10, Line 635: > nit: this probably isn't needed, right? Since we're going to retry the belo This duration is equal to "tablet_open_bootstrap_inject_latency_ms". The idea is that replica finishes bootstrap and tries to join consensus and finds out that it is not part of the consensus. -- 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: 11 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: Mon, 07 Oct 2019 06:43:04 +0000 Gerrit-HasComments: Yes
