Merge pull request #684 from drmingdrmer/60-refact-write

Refactor: Adds a new method `ensure_last_membership_committed()` to the `MembershipState`

Author: drmingdrmer

openraft/src/core/raft_core.rs

@@ -391,7 +391,7 @@ impl<C: RaftTypeConfig, N: RaftNetworkFactory<C>, S: RaftStorage<C>> RaftCore<C,
         turn_to_learner: bool,
         tx: RaftRespTx<ClientWriteResponse<C>, ClientWriteError<C::NodeId, C::Node>>,
     ) -> Result<(), Fatal<C::NodeId>> {
-        let res = self.engine.state.membership_state.next_membership(changes, turn_to_learner);
+        let res = self.engine.state.membership_state.create_updated_membership(changes, turn_to_learner);
         let new_membership = match res {
             Ok(x) => x,
             Err(e) => {

openraft/src/membership/membership_state.rs

@@ -80,26 +80,25 @@ where
         self.effective.membership.is_voter(id)
     }
 
-    /// Build a new membership config by applying changes to the current config.
+    /// Builds a new membership configuration by applying changes to the current configuration.
     ///
-    /// The removed voter is left in membership config as learner if `removed_to_learner` is true.
-    pub(crate) fn next_membership(
+    /// * `changes`: The changes to apply to the current membership configuration.
+    /// * `convert_removed_to_learner`: Indicates whether the removed voter should be left in the
+    ///   membership configuration as a learner.
+    ///
+    /// A Result containing the new membership configuration if the operation succeeds, or a
+    /// `ChangeMembershipError` if an error occurs.
+    ///
+    /// This function ensures that the cluster will have at least one voter in the new membership
+    /// configuration.
+    pub(crate) fn create_updated_membership(
         &self,
         changes: ChangeMembers<NID>,
-        removed_to_learner: bool,
+        convert_removed_to_learner: bool,
     ) -> Result<Membership<NID, N>, ChangeMembershipError<NID>> {
         let effective = self.effective();
-        let committed = self.committed();
 
-        if committed.log_id == effective.log_id {
-            // Ok: last membership(effective) is committed
-        } else {
-            return Err(InProgress {
-                committed: committed.log_id,
-                membership_log_id: effective.log_id,
-            }
-            .into());
-        }
+        self.ensure_last_membership_committed()?;
 
         let last = effective.membership.get_joint_config().last().unwrap();
         let new_voter_ids = changes.apply_to(last);
@@ -109,12 +108,31 @@ where
             return Err(EmptyMembership {}.into());
         }
 
-        let new_membership = effective.membership.next_safe(new_voter_ids, removed_to_learner)?;
+        let new_membership = effective.membership.next_safe(new_voter_ids, convert_removed_to_learner)?;
 
         tracing::debug!(?new_membership, "new membership config");
         Ok(new_membership)
     }
 
+    /// Ensures that the latest membership configuration has been committed.
+    ///
+    /// Returns Ok if the last membership configuration is committed, or an InProgress error
+    /// otherwise.
+    pub(crate) fn ensure_last_membership_committed(&self) -> Result<(), InProgress<NID>> {
+        let effective = self.effective();
+        let committed = self.committed();
+
+        if self.committed().log_id == self.effective().log_id {
+            // Ok: last membership(effective) is committed
+            Ok(())
+        } else {
+            Err(InProgress {
+                committed: committed.log_id,
+                membership_log_id: effective.log_id,
+            })
+        }
+    }
+
     /// Update membership state if the specified committed_log_id is greater than `self.effective`
     pub(crate) fn commit(&mut self, committed_log_id: &Option<LogId<NID>>) {
         if committed_log_id >= &self.effective().log_id {

openraft/src/membership/membership_state_test.rs

@@ -1,8 +1,14 @@
 use std::sync::Arc;
 
+use maplit::btreemap;
 use maplit::btreeset;
 
+use crate::error::ChangeMembershipError;
+use crate::error::EmptyMembership;
+use crate::error::InProgress;
+use crate::error::LearnerNotFound;
 use crate::testing::log_id;
+use crate::ChangeMembers;
 use crate::EffectiveMembership;
 use crate::Membership;
 use crate::MembershipState;
@@ -174,3 +180,66 @@ fn test_membership_state_truncate() -> anyhow::Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn test_membership_state_next_membership_not_committed() -> anyhow::Result<()> {
+    let new = || MembershipState::new(effmem(2, 2, m1()), effmem(3, 4, m123_345()));
+    let res = new().create_updated_membership(ChangeMembers::Add(btreeset! {1}), false);
+
+    assert_eq!(
+        Err(ChangeMembershipError::InProgress(InProgress {
+            committed: Some(log_id(2, 2)),
+            membership_log_id: Some(log_id(3, 4))
+        })),
+        res
+    );
+
+    Ok(())
+}
+
+#[test]
+fn test_membership_state_create_updated_membership_empty_voters() -> anyhow::Result<()> {
+    let new = || MembershipState::new(effmem(3, 4, m1()), effmem(3, 4, m1()));
+    let res = new().create_updated_membership(ChangeMembers::Remove(btreeset! {1}), false);
+
+    assert_eq!(Err(ChangeMembershipError::EmptyMembership(EmptyMembership {})), res);
+
+    Ok(())
+}
+
+#[test]
+fn test_membership_state_create_updated_membership_learner_not_found() -> anyhow::Result<()> {
+    let new = || MembershipState::new(effmem(3, 4, m1()), effmem(3, 4, m1()));
+    let res = new().create_updated_membership(ChangeMembers::Add(btreeset! {2}), false);
+
+    assert_eq!(
+        Err(ChangeMembershipError::LearnerNotFound(LearnerNotFound { node_id: 2 })),
+        res
+    );
+
+    Ok(())
+}
+
+#[test]
+fn test_membership_state_create_updated_membership_removed_to_learner() -> anyhow::Result<()> {
+    let new = || MembershipState::new(effmem(3, 4, m12()), effmem(3, 4, m123_345()));
+
+    // Do not leave removed voters as learner
+    let res = new().create_updated_membership(ChangeMembers::Remove(btreeset! {1,2}), false);
+    assert_eq!(
+        Ok(Membership::new(vec![btreeset! {3,4,5}], btreemap! {3=>(),4=>(),5=>()})),
+        res
+    );
+
+    // Leave removed voters as learner
+    let res = new().create_updated_membership(ChangeMembers::Remove(btreeset! {1,2}), true);
+    assert_eq!(
+        Ok(Membership::new(
+            vec![btreeset! {3,4,5}],
+            btreemap! {1=>(),2=>(),3=>(),4=>(),5=>()}
+        )),
+        res
+    );
+
+    Ok(())
+}