From 342d0de2b885388dfa5de64430384bd3275d3697 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BC=A0=E7=82=8E=E6=B3=BC?= Date: Wed, 1 Mar 2023 09:46:30 +0800 Subject: [PATCH] Change: rename variants in ChangeMembers, add `AddVoters` Rename `ChangeMembers::AddVoter` to `AddVoterIds`, because it just updates voter ids. Rename `ChangeMembers::RemoveVoter` to `RemoveVoters`. Add `ChangeMembers::AddVoters(BTreeMap)` to add voters with corresponding `Node`, i.e., it adds nodes as learners and update the voter-ids in a `Membership`. --- openraft/src/change_members.rs | 27 ++++++++++++- openraft/src/membership/change_handler.rs | 10 ++--- openraft/src/membership/membership.rs | 40 ++++++++++++++----- .../membership/t16_change_membership_cases.rs | 4 +- 4 files changed, 62 insertions(+), 19 deletions(-) diff --git a/openraft/src/change_members.rs b/openraft/src/change_members.rs index f87cee536..e07c27420 100644 --- a/openraft/src/change_members.rs +++ b/openraft/src/change_members.rs @@ -8,11 +8,34 @@ use crate::NodeId; #[derive(PartialEq, Eq)] #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize), serde(bound = ""))] pub enum ChangeMembers { - AddVoter(BTreeSet), - RemoveVoter(BTreeSet), + /// Upgrade learners to voters. + /// + /// The learners have to present or [`error::LearnerNotFound`](`crate::error::LearnerNotFound`) + /// error will be returned. + AddVoterIds(BTreeSet), + + /// Add voters with corresponding nodes. + AddVoters(BTreeMap), + + /// Remove voters, leave removed voters as learner or not. + RemoveVoters(BTreeSet), + + /// Replace voter ids with a new set. The node of every new voter has to already be a learner. ReplaceAllVoters(BTreeSet), + + /// Add nodes to membership, as learners. AddNodes(BTreeMap), + + /// Remove nodes from membership. + /// + /// If a node is still a voter, it returns + /// [`error::LearnerNotFound`](`crate::error::LearnerNotFound`) error. RemoveNodes(BTreeSet), + + /// Replace all nodes with a new set. + /// + /// Every voter has to have a corresponding node in the new + /// set, otherwise it returns [`error::LearnerNotFound`](`crate::error::LearnerNotFound`) error. ReplaceAllNodes(BTreeMap), } diff --git a/openraft/src/membership/change_handler.rs b/openraft/src/membership/change_handler.rs index d80e694e1..16ec946ba 100644 --- a/openraft/src/membership/change_handler.rs +++ b/openraft/src/membership/change_handler.rs @@ -99,7 +99,7 @@ mod tests { #[test] fn test_apply_not_committed() -> anyhow::Result<()> { let new = || MembershipState::new(effmem(2, 2, m1()), effmem(3, 4, m123_345())); - let res = new().change_handler().apply(ChangeMembers::AddVoter(btreeset! {1}), false); + let res = new().change_handler().apply(ChangeMembers::AddVoterIds(btreeset! {1}), false); assert_eq!( Err(ChangeMembershipError::InProgress(InProgress { @@ -115,7 +115,7 @@ mod tests { #[test] fn test_apply_empty_voters() -> anyhow::Result<()> { let new = || MembershipState::new(effmem(3, 4, m1()), effmem(3, 4, m1())); - let res = new().change_handler().apply(ChangeMembers::RemoveVoter(btreeset! {1}), false); + let res = new().change_handler().apply(ChangeMembers::RemoveVoters(btreeset! {1}), false); assert_eq!(Err(ChangeMembershipError::EmptyMembership(EmptyMembership {})), res); @@ -125,7 +125,7 @@ mod tests { #[test] fn test_apply_learner_not_found() -> anyhow::Result<()> { let new = || MembershipState::new(effmem(3, 4, m1()), effmem(3, 4, m1())); - let res = new().change_handler().apply(ChangeMembers::AddVoter(btreeset! {2}), false); + let res = new().change_handler().apply(ChangeMembers::AddVoterIds(btreeset! {2}), false); assert_eq!( Err(ChangeMembershipError::LearnerNotFound(LearnerNotFound { node_id: 2 })), @@ -140,14 +140,14 @@ mod tests { let new = || MembershipState::new(effmem(3, 4, m12()), effmem(3, 4, m123_345())); // Do not leave removed voters as learner - let res = new().change_handler().apply(ChangeMembers::RemoveVoter(btreeset! {1,2}), false); + let res = new().change_handler().apply(ChangeMembers::RemoveVoters(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().change_handler().apply(ChangeMembers::RemoveVoter(btreeset! {1,2}), true); + let res = new().change_handler().apply(ChangeMembers::RemoveVoters(btreeset! {1,2}), true); assert_eq!( Ok(Membership::new( vec![btreeset! {3,4,5}], diff --git a/openraft/src/membership/membership.rs b/openraft/src/membership/membership.rs index e7fec4cb1..e2a8d2e76 100644 --- a/openraft/src/membership/membership.rs +++ b/openraft/src/membership/membership.rs @@ -402,14 +402,22 @@ where ) -> Result> { tracing::debug!(change = debug(&change), "{}", func_name!()); - let last = self.get_joint_config().last().unwrap(); + let last = self.get_joint_config().last().unwrap().clone(); let new_membership = match change { - ChangeMembers::AddVoter(add_voter_ids) => { + ChangeMembers::AddVoterIds(add_voter_ids) => { let new_voter_ids = last.union(&add_voter_ids).copied().collect::>(); self.next_coherent(new_voter_ids, retain) } - ChangeMembers::RemoveVoter(remove_voter_ids) => { + ChangeMembers::AddVoters(add_voters) => { + // Add nodes without overriding existent + self.nodes = Self::extend_nodes(self.nodes, &add_voters); + + let add_voter_ids = add_voters.keys().copied().collect::>(); + let new_voter_ids = last.union(&add_voter_ids).copied().collect::>(); + self.next_coherent(new_voter_ids, retain) + } + ChangeMembers::RemoveVoters(remove_voter_ids) => { let new_voter_ids = last.difference(&remove_voter_ids).copied().collect::>(); self.next_coherent(new_voter_ids, retain) } @@ -480,7 +488,7 @@ mod tests { // Add: no such learner { - let res = m().change(ChangeMembers::AddVoter(btreeset! {4}), true); + let res = m().change(ChangeMembers::AddVoterIds(btreeset! {4}), true); assert_eq!( Err(ChangeMembershipError::LearnerNotFound(LearnerNotFound { node_id: 4 })), res @@ -489,7 +497,7 @@ mod tests { // Add: ok { - let res = m().change(ChangeMembers::AddVoter(btreeset! {3}), true); + let res = m().change(ChangeMembers::AddVoterIds(btreeset! {3}), true); assert_eq!( Ok(Membership:: { configs: vec![btreeset! {1,2}, btreeset! {1,2,3}], @@ -499,9 +507,21 @@ mod tests { ); } + // AddVoters + { + let res = m().change(ChangeMembers::AddVoters(btreemap! {5=>()}), true); + assert_eq!( + Ok(Membership:: { + configs: vec![btreeset! {1,2}, btreeset! {1,2,5}], + nodes: btreemap! {1=>(),2=>(),3=>(),5=>()} + }), + res + ); + } + // Remove: no such voter { - let res = m().change(ChangeMembers::RemoveVoter(btreeset! {5}), true); + let res = m().change(ChangeMembers::RemoveVoters(btreeset! {5}), true); assert_eq!( Ok(Membership:: { configs: vec![btreeset! {1,2}], @@ -513,13 +533,13 @@ mod tests { // Remove: become empty { - let res = m().change(ChangeMembers::RemoveVoter(btreeset! {1,2}), true); + let res = m().change(ChangeMembers::RemoveVoters(btreeset! {1,2}), true); assert_eq!(Err(ChangeMembershipError::EmptyMembership(EmptyMembership {})), res); } // Remove: OK retain { - let res = m().change(ChangeMembers::RemoveVoter(btreeset! {1}), true); + let res = m().change(ChangeMembers::RemoveVoters(btreeset! {1}), true); assert_eq!( Ok(Membership:: { configs: vec![btreeset! {1,2}, btreeset! {2}], @@ -531,7 +551,7 @@ mod tests { // Remove: OK, not retain; learner not removed { - let res = m().change(ChangeMembers::RemoveVoter(btreeset! {1}), false); + let res = m().change(ChangeMembers::RemoveVoters(btreeset! {1}), false); assert_eq!( Ok(Membership:: { configs: vec![btreeset! {1,2}, btreeset! {2}], @@ -547,7 +567,7 @@ mod tests { configs: vec![btreeset! {1,2}, btreeset! {2}], nodes: btreemap! {1=>(),2=>(),3=>()}, }; - let res = mem.change(ChangeMembers::RemoveVoter(btreeset! {1}), false); + let res = mem.change(ChangeMembers::RemoveVoters(btreeset! {1}), false); assert_eq!( Ok(Membership:: { configs: vec![btreeset! {2}], diff --git a/openraft/tests/membership/t16_change_membership_cases.rs b/openraft/tests/membership/t16_change_membership_cases.rs index da85073cb..9f148d936 100644 --- a/openraft/tests/membership/t16_change_membership_cases.rs +++ b/openraft/tests/membership/t16_change_membership_cases.rs @@ -244,7 +244,7 @@ async fn change_from_to(old: BTreeSet, change_members: BTreeSet, add: &[MemNodeId]) -> anyhow::Result<()> { - let change = ChangeMembers::AddVoter(add.iter().copied().collect()); + let change = ChangeMembers::AddVoterIds(add.iter().copied().collect()); let mes = format!("from {:?} {:?}", old, change); @@ -312,7 +312,7 @@ async fn change_by_add(old: BTreeSet, add: &[MemNodeId]) -> anyhow::R #[tracing::instrument(level = "debug")] async fn change_by_remove(old: BTreeSet, remove: &[MemNodeId]) -> anyhow::Result<()> { - let change = ChangeMembers::RemoveVoter(remove.iter().copied().collect()); + let change = ChangeMembers::RemoveVoters(remove.iter().copied().collect()); let mes = format!("from {:?} {:?}", old, change);