fix: client_read has using wrong quorum=majority-1

drmingdrmer · drmingdrmer · commit d60f1e852d3e · 2021-07-02T13:19:09.000+08:00
diff --git 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<D>, S: RaftStorage<D, R>
     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<D>, S: RaftStorage<D, R>
             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<D>, S: RaftStorage<D, R>
 
         // 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
@@ -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
@@ -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
@@ -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
@@ -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(())
 }

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+pub fn majority_of(n: usize) -> usize {`
	`2`	`+ n / 2 + 1`
	`3`	`+}`