Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14300 )
Change subject: KUDU-2800 Test long bootstrapping tablet replicas ...................................................................... Patch Set 11: (12 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: } : workload.StopAndJoin(); : : 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( : > I think if we shutdown leader, it may not quickly propagate across the clus I think GetLeaderReplicaWithRetries() handles those cases as well. Or you have some evidence it fails doing so? http://gerrit.cloudera.org:8080/#/c/14300/6/src/kudu/integration-tests/raft_config_change-itest.cc@628 PS6, Line 628: : > We give that replica time equal to tablet_open_bootstrap_inject_latency_ms OK, but given the exact timings, might it be a source for flakiness? Or, put it another way: what might be different in the ASSERT_EVENTUALLY below if this delay is not in place? 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::pair; > warning: using decl 'pair' is unused [misc-unused-using-decls] It makes sense to address the issues reported by the Tidy bot. http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@465 PS11, Line 465: void ValidateConsensusStateChanged(const string& added_replica_uuid, Add short doc for the parameters. http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@465 PS11, Line 465: void ValidateConsensusStateChanged(const string& added_replica_uuid, : const string& removed_replica_uuid, : const MonoDelta& timeout) { nit: align the parameters This might be helpful https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions for details. http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@476 PS11, Line 476: void SetUpCluster(bool add_flags_for_log_rolls, Document the parameters. http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@477 PS11, Line 477: any_replica_sever any_replica_server ? http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@480 PS11, Line 480: const vector<string> kMasterFlags { : "--tserver_unresponsive_timeout_ms=3000" : }; : vector<string> ts_flags { : "--consensus_rpc_timeout_ms=2000", : "--follower_unavailable_considered_failed_sec=3" : }; nit: document the reasoning behind adding these extra flags and relationship between their values. http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@511 PS11, Line 511: const MonoDelta kTimeout = MonoDelta::FromSeconds(30); nit: it seems this also can be factored out into a member of the SlowTabletBootstrapTest class. http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/integration-tests/raft_config_change-itest.cc@531 PS11, Line 531: // it falls behind and is removed from consensus nit: add period http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/tserver/ts_tablet_manager.cc@141 PS11, Line 141: tablet_open_bootstrap_inject_latency_ms nit: why not simply tablet_bootstrap_inject_latency_ms ? http://gerrit.cloudera.org:8080/#/c/14300/11/src/kudu/tserver/ts_tablet_manager.cc@142 PS11, Line 142: "Injects latency into the tablet bootstrapping." nit: add a space -- 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 14:05:30 +0000 Gerrit-HasComments: Yes
