Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14260 )
Change subject: [consensus] KUDU-2947 fix voting in case of slow WAL ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/14260/1/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: http://gerrit.cloudera.org:8080/#/c/14260/1/src/kudu/consensus/raft_consensus.cc@1437 PS1, Line 1437: : // Also prohibit voting for anyone for the minimum election timeout. : withhold_votes_until_ = MonoTime::Now() + MinimumElectionTimeout(); Could you decompose this pattern into a function with a descriptive name? http://gerrit.cloudera.org:8080/#/c/14260/1/src/kudu/integration-tests/raft_consensus_election-itest.cc File src/kudu/integration-tests/raft_consensus_election-itest.cc: http://gerrit.cloudera.org:8080/#/c/14260/1/src/kudu/integration-tests/raft_consensus_election-itest.cc@581 PS1, Line 581: const auto kMaxMissedHeartbeatPeriods = 3; This is the default value. http://gerrit.cloudera.org:8080/#/c/14260/1/src/kudu/integration-tests/raft_consensus_election-itest.cc@595 PS1, Line 595: ASSERT_OK(GetLeaderReplicaWithRetries(tablet_id_, &leader)); Leadership can fluctuate after this call completes. How will the test respond if it does? http://gerrit.cloudera.org:8080/#/c/14260/1/src/kudu/integration-tests/raft_consensus_election-itest.cc@614 PS1, Line 614: if (leader_tserver_idx == idx) { : ASSERT_OK(cluster_->SetFlag(ts, "log_inject_latency_ms_mean", : std::to_string(2 * kHeartbeatIntervalMs * kMaxMissedHeartbeatPeriods))); : ASSERT_OK(cluster_->SetFlag(ts, "log_inject_latency_ms_stddev", "0")); : ASSERT_OK(cluster_->SetFlag(ts, "log_inject_latency", "true")); : } else { : ASSERT_OK(cluster_->SetFlag(ts, "log_inject_latency_ms_mean", : std::to_string(kHeartbeatIntervalMs * kMaxMissedHeartbeatPeriods))); : ASSERT_OK(cluster_->SetFlag(ts, "log_inject_latency_ms_stddev", "0")); : ASSERT_OK(cluster_->SetFlag(ts, "log_inject_latency", "true")); : } This can be condensed somewhat: - The second and third SetFlag() calls can be pulled out of the if statement. - The first can use a ternary to avoid duplicating the call. http://gerrit.cloudera.org:8080/#/c/14260/1/src/kudu/integration-tests/raft_consensus_election-itest.cc@664 PS1, Line 664: ASSERT_FALSE(s.ok()) << "vote unexpectedly granted while leader is alive"; Can we assert on a particular status code? -- To view, visit http://gerrit.cloudera.org:8080/14260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7c061b498e727a1a11e94e03c55530eeebfdf8dd Gerrit-Change-Number: 14260 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Comment-Date: Thu, 19 Sep 2019 04:10:21 +0000 Gerrit-HasComments: Yes
