From d60f1e852d3e5b9455589593067599d261f695b2 Mon Sep 17 00:00:00 2001 From: drdr xp Date: Fri, 2 Jul 2021 13:19:09 +0800 Subject: [PATCH] fix: client_read has using wrong quorum=majority-1 --- async-raft/src/core/client.rs | 21 +++++++++++---------- async-raft/src/core/replication.rs | 5 +++-- async-raft/src/lib.rs | 1 + async-raft/src/quorum.rs | 3 +++ async-raft/tests/client_reads.rs | 14 ++++++++++++++ 5 files changed, 32 insertions(+), 12 deletions(-) create mode 100644 async-raft/src/quorum.rs diff --git a/async-raft/src/core/client.rs b/async-raft/src/core/client.rs index d4886068c..fa8d2ec5c 100644 --- a/async-raft/src/core/client.rs +++ b/async-raft/src/core/client.rs @@ -14,6 +14,7 @@ use crate::error::ClientReadError; use crate::error::ClientWriteError; use crate::error::RaftError; use crate::error::RaftResult; +use crate::quorum; use crate::raft::AppendEntriesRequest; use crate::raft::ClientReadResponseTx; use crate::raft::ClientWriteRequest; @@ -126,17 +127,16 @@ impl<'a, D: AppData, R: AppDataResponse, N: RaftNetwork, S: RaftStorage pub(super) async fn handle_client_read_request(&mut self, tx: ClientReadResponseTx) { // Setup sentinel values to track when we've received majority confirmation of leadership. let mut c0_confirmed = 0usize; - let len_members = self.core.membership.members.len(); // Will never be zero, as we don't allow it when proposing config changes. - let c0_needed: usize = if (len_members % 2) == 0 { - (len_members / 2) - 1 - } else { - len_members / 2 - }; + // Will never be zero, as we don't allow it when proposing config changes. + let len_members = self.core.membership.members.len(); + + let c0_needed = quorum::majority_of(len_members); + let mut c1_confirmed = 0usize; let mut c1_needed = 0usize; if let Some(joint_members) = &self.core.membership.members_after_consensus { let len = joint_members.len(); // Will never be zero, as we don't allow it when proposing config changes. - c1_needed = if (len % 2) == 0 { (len / 2) - 1 } else { len / 2 }; + c1_needed = quorum::majority_of(len); } // Increment confirmations for self, including post-joint-consensus config if applicable. @@ -152,7 +152,7 @@ impl<'a, D: AppData, R: AppDataResponse, N: RaftNetwork, S: RaftStorage c1_confirmed += 1; } - // If we already have all needed confirmations — which would be the case for singlenode + // If we already have all needed confirmations — which would be the case for single node // clusters — then respond. if c0_confirmed >= c0_needed && c1_confirmed >= c1_needed { let _ = tx.send(Ok(())); @@ -186,14 +186,15 @@ impl<'a, D: AppData, R: AppDataResponse, N: RaftNetwork, S: RaftStorage // Handle responses as they return. while let Some(res) = pending.next().await { + // TODO(xp): if receives error about a higher term, it should stop at once? let (target, data) = match res { Ok(Ok(res)) => res, Ok(Err((target, err))) => { - tracing::error!({target, error=%err}, "timeout while confirming leadership for read request"); + tracing::error!(target, error=%err, "timeout while confirming leadership for read request"); continue; } Err((target, err)) => { - tracing::error!({ target }, "{}", err); + tracing::error!(target, "{}", err); continue; } }; diff --git a/async-raft/src/core/replication.rs b/async-raft/src/core/replication.rs index a3b21fc42..05b861c88 100644 --- a/async-raft/src/core/replication.rs +++ b/async-raft/src/core/replication.rs @@ -10,6 +10,7 @@ use crate::core::SnapshotState; use crate::core::State; use crate::core::UpdateCurrentLeader; use crate::error::RaftResult; +use crate::quorum; use crate::replication::RaftEvent; use crate::replication::ReplicaEvent; use crate::replication::ReplicationStream; @@ -348,8 +349,8 @@ fn calculate_new_commit_index(mut entries: Vec<(u64, u64)>, current_commit: u64, entries.sort_unstable_by(|a, b| a.0.cmp(&b.0)); - let quorum = entries.len() / 2 + 1; - let offset = entries.len() - quorum; + let majority = quorum::majority_of(entries.len()); + let offset = entries.len() - majority; let new_val = entries[offset]; diff --git a/async-raft/src/lib.rs b/async-raft/src/lib.rs index e35265e3d..9fae1c387 100644 --- a/async-raft/src/lib.rs +++ b/async-raft/src/lib.rs @@ -7,6 +7,7 @@ pub mod metrics; #[cfg(test)] mod metrics_wait_test; pub mod network; +mod quorum; pub mod raft; mod raft_types; mod replication; diff --git a/async-raft/src/quorum.rs b/async-raft/src/quorum.rs new file mode 100644 index 000000000..da4a5075b --- /dev/null +++ b/async-raft/src/quorum.rs @@ -0,0 +1,3 @@ +pub fn majority_of(n: usize) -> usize { + n / 2 + 1 +} diff --git a/async-raft/tests/client_reads.rs b/async-raft/tests/client_reads.rs index aeb95f61f..3f79a3d42 100644 --- a/async-raft/tests/client_reads.rs +++ b/async-raft/tests/client_reads.rs @@ -50,8 +50,22 @@ async fn client_reads() -> Result<()> { .client_read(leader) .await .unwrap_or_else(|_| panic!("expected client_read to succeed for cluster leader {}", leader)); + router.client_read(1).await.expect_err("expected client_read on follower node 1 to fail"); router.client_read(2).await.expect_err("expected client_read on follower node 2 to fail"); + tracing::info!("--- isolate node 1 then client read should work"); + + router.isolate_node(1).await; + router.client_read(leader).await?; + + tracing::info!("--- isolate node 2 then client read should fail"); + + router.isolate_node(2).await; + let rst = router.client_read(leader).await; + tracing::debug!(?rst, "client_read with majority down"); + + assert!(rst.is_err()); + Ok(()) }