You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by ac...@apache.org on 2026/10/17 00:44:39 UTC
(kudu) branch master updated: KUDU-3816 [consensus] Send the new leader's NO_OP without waiting for a heartbeat
This is an automated email from the ASF dual-hosted git repository.
asf-gitbox-commits pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new a0409299d KUDU-3816 [consensus] Send the new leader's NO_OP without waiting for a heartbeat
a0409299d is described below
commit a0409299d44318756156ca8760812b1fdfadf291
Author: Abhishek Chennaka <ac...@apache.org>
AuthorDate: Sun Oct 11 12:30:10 2026 -0700
KUDU-3816 [consensus] Send the new leader's NO_OP without waiting for a heartbeat
A new leader now signals its peers when it appends its NO_OP. This will
ensure the the clean time is initialized and the tablet's replicas
serve the scans independent of --raft_heartbeat_interval_ms.
The regression test for KUDU-2147 has been updated according to the new behavior.
AdminCliTest.TestRebuildTables now rebuilds TestTable in add mode with
its actual replication factor of 3. With the default of 1 the master
evicts the extra replicas after the restart which this change makes
happen before the test sees all 3 replicas.
The repro.cc attached at KUDU_3747 doesn't show any latency with this patch
in place.
Change-Id: I1c1c4e448a1bf0142d32ee7e2721d304e444f9d8
Reviewed-on: http://gerrit.cloudera.org:8080/25101
Reviewed-by: Alexey Serbin <al...@apache.org>
Tested-by: Abhishek Chennaka <ac...@cloudera.com>
---
src/kudu/consensus/raft_consensus.cc | 24 ++++++++----
src/kudu/consensus/raft_consensus.h | 1 -
src/kudu/consensus/raft_consensus_quorum-test.cc | 33 ++++++++++++++++
.../integration-tests/raft_config_change-itest.cc | 45 +++++++++++++++++-----
src/kudu/tools/kudu-admin-test.cc | 8 +++-
5 files changed, 92 insertions(+), 19 deletions(-)
diff --git a/src/kudu/consensus/raft_consensus.cc b/src/kudu/consensus/raft_consensus.cc
index b39a653aa..434440933 100644
--- a/src/kudu/consensus/raft_consensus.cc
+++ b/src/kudu/consensus/raft_consensus.cc
@@ -417,15 +417,19 @@ Status RaftConsensus::EmulateElectionForTests() {
"tablet", options_.tablet_id);
ThreadRestrictions::AssertWaitAllowed();
- std::lock_guard l(lock_);
- RETURN_NOT_OK(CheckRunningUnlocked());
+ {
+ std::lock_guard l(lock_);
+ RETURN_NOT_OK(CheckRunningUnlocked());
- LOG_WITH_PREFIX_UNLOCKED(INFO) << "Emulating election...";
+ LOG_WITH_PREFIX_UNLOCKED(INFO) << "Emulating election...";
- // Assume leadership of new term.
- RETURN_NOT_OK(HandleTermAdvanceUnlocked(CurrentTermUnlocked() + 1));
- SetLeaderUuidUnlocked(peer_uuid());
- return BecomeLeaderUnlocked();
+ // Assume leadership of new term.
+ RETURN_NOT_OK(HandleTermAdvanceUnlocked(CurrentTermUnlocked() + 1));
+ SetLeaderUuidUnlocked(peer_uuid());
+ RETURN_NOT_OK(BecomeLeaderUnlocked());
+ } // Release lock before signaling request.
+ peer_manager_->SignalRequest();
+ return Status::OK();
}
namespace {
@@ -2816,6 +2820,12 @@ void RaftConsensus::DoElectionCallback(ElectionReason reason, const ElectionResu
// It races with the above state check.
// This could be a problem during tablet deletion.
CHECK_OK(BecomeLeaderUnlocked());
+
+ // Send the NO_OP to the followers right away instead of waiting for the
+ // first heartbeat which is up to --raft_heartbeat_interval_ms away. This
+ // will ensure MVCC time is initialized and the replicas can serve scans.
+ l.unlock();
+ peer_manager_->SignalRequest();
}
}
diff --git a/src/kudu/consensus/raft_consensus.h b/src/kudu/consensus/raft_consensus.h
index 8fc3dac25..84dc11cdf 100644
--- a/src/kudu/consensus/raft_consensus.h
+++ b/src/kudu/consensus/raft_consensus.h
@@ -38,7 +38,6 @@
#include "kudu/consensus/opid.pb.h"
#include "kudu/consensus/ref_counted_replicate.h"
#include "kudu/gutil/macros.h"
-#include "kudu/gutil/port.h"
#include "kudu/gutil/ref_counted.h"
#include "kudu/tablet/metadata.pb.h"
#include "kudu/tserver/tserver.pb.h"
diff --git a/src/kudu/consensus/raft_consensus_quorum-test.cc b/src/kudu/consensus/raft_consensus_quorum-test.cc
index 51d16958e..24d4f56bd 100644
--- a/src/kudu/consensus/raft_consensus_quorum-test.cc
+++ b/src/kudu/consensus/raft_consensus_quorum-test.cc
@@ -886,6 +886,39 @@ TEST_F(RaftConsensusQuorumTest, TestLeaderHeartbeats) {
VerifyLogs(2, 0, 1);
}
+// A newly elected leader must replicate its NO_OP right away rather than on
+// its first heartbeat as until then MVCC clean time isn't initialized and the
+// tablet's replicas reject scans. With a heartbeat interval far longer than
+// the commit wait below the NO_OP can only get committed in time if the leader
+// sends it on becoming leader.
+TEST_F(RaftConsensusQuorumTest, TestNewLeaderReplicatesNoOpWithoutHeartbeat) {
+ constexpr int kFollower0Idx = 0;
+ constexpr int kFollower1Idx = 1;
+ constexpr int kLeaderIdx = 2;
+ FLAGS_raft_heartbeat_interval_ms = 60 * 1000;
+
+ ASSERT_OK(BuildAndStartConfig(3));
+
+ // The initial config isn't a log entry (its opid_index is kInvalidOpIdIndex),
+ // so the NO_OP of the leader's term is the first op in the log.
+ constexpr int64_t kNoOpIndex = 1;
+ NO_FATALS(WaitForCommitIfNotAlreadyPresent(kNoOpIndex, kLeaderIdx, kLeaderIdx));
+ NO_FATALS(WaitForCommitIfNotAlreadyPresent(kNoOpIndex, kFollower0Idx, kLeaderIdx));
+ NO_FATALS(WaitForCommitIfNotAlreadyPresent(kNoOpIndex, kFollower1Idx, kLeaderIdx));
+
+ // Make sure the op committed above is the NO_OP and not some other op. The
+ // next op replicated by the leader must directly follow it.
+ scoped_refptr<ConsensusRound> round;
+ ASSERT_OK(AppendDummyMessage(kLeaderIdx, &round));
+ ASSERT_OK(WaitForReplicate(round.get()));
+ ASSERT_OK(CommitDummyMessage(kLeaderIdx, round.get()));
+ ASSERT_EQ(kNoOpIndex + 1, round->id().index());
+ WaitForReplicateIfNotAlreadyPresent(round->id(), kFollower0Idx);
+ WaitForReplicateIfNotAlreadyPresent(round->id(), kFollower1Idx);
+
+ VerifyLogs(kLeaderIdx, kFollower0Idx, kFollower1Idx);
+}
+
// After creating the initial configuration, this test writes a small sequence
// of messages to the initial leader. It then shuts down the current
// leader, makes another peer become leader and writes a sequence of
diff --git a/src/kudu/integration-tests/raft_config_change-itest.cc b/src/kudu/integration-tests/raft_config_change-itest.cc
index ccb530284..65cfbedba 100644
--- a/src/kudu/integration-tests/raft_config_change-itest.cc
+++ b/src/kudu/integration-tests/raft_config_change-itest.cc
@@ -95,12 +95,14 @@ class RaftConfigChangeITest : public ExternalMiniClusterITestBase {
// heartbeat intervals to followers to be able to carefully control the
// sequence of events in this test.
// 2. Evict a follower from the config.
-// 3. Immediately start an election on the remaining follower in the config.
-// We know that this follower should win the election because in order to
-// commit a removal from 3 voters, the removal op must be replicated to both
-// of the two remaining voters in the config.
+// 3. Make the initial leader reject consensus updates, then immediately start
+// an election on the remaining follower in the config. We know that this
+// follower should win the election because in order to commit a removal
+// from 3 voters, the removal op must be replicated to both of the two
+// remaining voters in the config.
// 4. The master will get the config change heartbeat from the initial leader,
-// which will indicate a new term and no current leader.
+// which will indicate a new term and no current leader. Once the master has
+// it, the initial leader is allowed to accept consensus updates again.
// 5. The master will then get a heartbeat from the new leader to report that
// it's now the leader of the config.
// 6. Once the master knows who the new leader is, the master will instruct the
@@ -114,10 +116,9 @@ class RaftConfigChangeITest : public ExternalMiniClusterITestBase {
TEST_F(RaftConfigChangeITest, TestKudu2147) {
SKIP_IF_SLOW_NOT_ALLOWED();
const MonoDelta kTimeout = MonoDelta::FromSeconds(30);
- // Slow down leader heartbeats so that in the explicit election below, the
- // second leader does not immediately heartbeat to the initial leader. If
- // that occurs, the initial leader will not report to the master that there
- // is currently no leader.
+ // Slow down leader heartbeats so that the initial leader, while it rejects
+ // consensus updates from the new leader below, does not detect a leader
+ // failure and start an election of its own.
NO_FATALS(StartCluster({"--raft_heartbeat_interval_ms=3000"}));
// Create a new table.
@@ -142,14 +143,19 @@ TEST_F(RaftConfigChangeITest, TestKudu2147) {
ASSERT_OK(WaitForServersToAgree(kTimeout, ts_map_, tablet_id, 1));
// Retry as needed to counter normal leader election activity.
+ string initial_leader_uuid;
ASSERT_EVENTUALLY([&] {
// Find initial leader.
TServerDetails* leader;
ASSERT_OK(FindTabletLeader(ts_map_, tablet_id, kTimeout, &leader));
ASSERT_OK(WaitForOpFromCurrentTerm(leader, tablet_id, COMMITTED_OPID, kTimeout));
+ initial_leader_uuid = leader->uuid();
vector<TServerDetails*> followers;
for (int i = 0; i < cluster_->num_tablet_servers(); i++) {
+ // Undo the update rejection from a previous attempt, if any.
+ ASSERT_OK(cluster_->SetFlag(cluster_->tablet_server(i),
+ "follower_reject_update_consensus_requests", "false"));
// Dynamically set the latency injection flag to induce TS -> master
// heartbeat delays. The leader delays its master heartbeats by 2 sec
// each time; followers delay their master heartbeats by even longer.
@@ -168,6 +174,12 @@ TEST_F(RaftConfigChangeITest, TestKudu2147) {
// an election on another follower immediately thereafter.
ASSERT_OK(itest::RemoveServer(leader, tablet_id, followers[0], kTimeout));
+ // A new leader sends its NO_OP to the followers right after the election.
+ // Have the initial leader reject it so the initial leader doesn't learn
+ // who the new leader is before reporting to the master.
+ ASSERT_OK(cluster_->SetFlag(cluster_->tablet_server_by_uuid(leader->uuid()),
+ "follower_reject_update_consensus_requests", "true"));
+
// Immediately start an election on the remaining follower. This will cause
// the initial leader's term to rev and it will have to step down. When it
// sends a tablet report to the master with the new configuration excluding
@@ -175,6 +187,21 @@ TEST_F(RaftConfigChangeITest, TestKudu2147) {
ASSERT_OK(itest::StartElection(followers[1], tablet_id, kTimeout));
});
+ // Wait for the master to get that report with no leader for the tablet.
+ ASSERT_EVENTUALLY([&] {
+ master::GetTableLocationsResponsePB locations;
+ ASSERT_OK(GetTableLocations(cluster_->master_proxy(), TestWorkload::kDefaultTableName,
+ kTimeout, VOTER_REPLICA, /*table_id=*/nullopt, &locations));
+ ASSERT_EQ(1, locations.tablet_locations_size());
+ const auto& replicas = locations.tablet_locations(0).interned_replicas();
+ ASSERT_EQ(2, replicas.size());
+ for (const auto& replica : replicas) {
+ ASSERT_NE(RaftPeerPB::LEADER, replica.role());
+ }
+ });
+ ASSERT_OK(cluster_->SetFlag(cluster_->tablet_server_by_uuid(initial_leader_uuid),
+ "follower_reject_update_consensus_requests", "false"));
+
// Wait until the master re-adds the evicted replica and it is back up and
// running. If the master hit KUDU-2147, this would fail because the master
// would be unable to add the removed server back, and that replica would be
diff --git a/src/kudu/tools/kudu-admin-test.cc b/src/kudu/tools/kudu-admin-test.cc
index 6fd36ca94..324e10f88 100644
--- a/src/kudu/tools/kudu-admin-test.cc
+++ b/src/kudu/tools/kudu-admin-test.cc
@@ -4284,9 +4284,13 @@ TEST_F(AdminCliTest, TestRebuildTables) {
kTable1);
string stdout2;
string stderr2;
- // Rebuild kTable1 in add mode.
+ // Rebuild kTable1 in add mode. Its tablets have 3 replicas since the rebuild
+ // above so keep the replication factor at 3. If not, the master evicts the
+ // excess replicas after the restart and WaitForTSAndReplicas() below can't
+ // find 3 replicas.
ASSERT_OK(RunKuduTool(RebuildMasterCmd(*cluster_, FLAGS_num_tablet_servers,
- /*is_secure*/false, /*log_to_stderr*/true, kTable1),
+ /*is_secure*/false, /*log_to_stderr*/true,
+ kTable1, /*default_replica_num*/3),
&stdout2, &stderr2));
ASSERT_STR_NOT_CONTAINS(stderr2, "must be empty");
ASSERT_STR_CONTAINS(stdout2,