From 9df97bfbaa91af36344b4a199f6d5d426a6d0e0f Mon Sep 17 00:00:00 2001 From: lichuang Date: Tue, 25 Jan 2022 11:41:14 +0800 Subject: [PATCH 01/10] Refactor: add option in function change_membership, turn not exist member into learner, or remove it --- openraft/src/core/admin.rs | 2 + openraft/src/core/mod.rs | 17 ++++-- openraft/src/membership/membership.rs | 21 ++++++++ openraft/src/membership/membership_test.rs | 2 + openraft/src/raft.rs | 34 ++++++++++-- openraft/tests/fixtures/mod.rs | 15 +++++- openraft/tests/membership/main.rs | 4 ++ .../tests/membership/t20_change_membership.rs | 53 +++++++++++++++++++ 8 files changed, 140 insertions(+), 8 deletions(-) diff --git a/openraft/src/core/admin.rs b/openraft/src/core/admin.rs index da62cf319..6e30b0d85 100644 --- a/openraft/src/core/admin.rs +++ b/openraft/src/core/admin.rs @@ -142,6 +142,7 @@ impl<'a, D: AppData, R: AppDataResponse, N: RaftNetwork, S: RaftStorage &mut self, members: BTreeSet, blocking: bool, + turn_to_learner: bool, tx: RaftRespTx, ClientWriteError>, ) -> Result<(), StorageError> { // Ensure cluster will have at least one node. @@ -166,6 +167,7 @@ impl<'a, D: AppData, R: AppDataResponse, N: RaftNetwork, S: RaftStorage let curr = self.core.effective_membership.membership.clone(); let new_members = members.difference(curr.all_members()); let mut new_config = curr.next_safe(members.clone()); + new_config.set_turn_to_learner(turn_to_learner); tracing::debug!(?new_config, "new_config"); // Check the proposed config for any new nodes. If ALL new nodes already have replication diff --git a/openraft/src/core/mod.rs b/openraft/src/core/mod.rs index e7082d12d..2d8dfd39f 100644 --- a/openraft/src/core/mod.rs +++ b/openraft/src/core/mod.rs @@ -450,7 +450,13 @@ impl, S: RaftStorage> Ra self.set_target_state(State::Follower); } } else { - self.set_target_state(State::Learner); + if self.effective_membership.membership.is_turn_to_learner() { + self.set_target_state(State::Learner); + tracing::debug!("node {} turn to learner", self.id); + } else { + self.set_target_state(State::Shutdown); + tracing::debug!("node {} has been removed,shutdowning...", self.id); + } } } @@ -836,8 +842,13 @@ impl<'a, D: AppData, R: AppDataResponse, N: RaftNetwork, S: RaftStorage RaftMsg::AddLearner { id, tx, blocking } => { self.add_learner(id, tx, blocking).await; } - RaftMsg::ChangeMembership { members, blocking, tx } => { - self.change_membership(members, blocking, tx).await?; + RaftMsg::ChangeMembership { + members, + blocking, + turn_to_learner, + tx, + } => { + self.change_membership(members, blocking, turn_to_learner, tx).await?; } }; diff --git a/openraft/src/membership/membership.rs b/openraft/src/membership/membership.rs index afdfccda9..ba28d9214 100644 --- a/openraft/src/membership/membership.rs +++ b/openraft/src/membership/membership.rs @@ -27,6 +27,9 @@ pub struct Membership { /// Cache of all node ids. all_members: BTreeSet, + + /// default: true + turn_to_learner: bool, } impl MessageSummary for Membership { @@ -55,41 +58,49 @@ impl MessageSummary for Membership { impl Membership { pub fn new_single(members: BTreeSet) -> Self { let configs = vec![members]; + let turn_to_learner = true; let all_members = Self::build_all_members(&configs); let learners = BTreeSet::new(); Membership { learners, configs, all_members, + turn_to_learner, } } pub fn new_single_with_learners(members: BTreeSet, learners: BTreeSet) -> Self { let configs = vec![members]; let all_members = Self::build_all_members(&configs); + let turn_to_learner = true; Membership { learners, configs, all_members, + turn_to_learner, } } pub fn new_multi(configs: Vec>) -> Self { let all_members = Self::build_all_members(&configs); let learners = BTreeSet::new(); + let turn_to_learner = true; Membership { learners, configs, all_members, + turn_to_learner, } } pub fn new_multi_with_learners(configs: Vec>, learners: BTreeSet) -> Self { let all_members = Self::build_all_members(&configs); + let turn_to_learner = true; Membership { learners, configs, all_members, + turn_to_learner, } } @@ -99,13 +110,23 @@ impl Membership { learners.insert(*id); let configs = self.configs.clone(); let all_members = Self::build_all_members(&self.configs); + let turn_to_learner = self.turn_to_learner.clone(); Membership { learners, configs, all_members, + turn_to_learner, } } + pub fn set_turn_to_learner(&mut self, turn_to_learner: bool) { + self.turn_to_learner = turn_to_learner; + } + + pub fn is_turn_to_learner(&self) -> bool { + self.turn_to_learner + } + pub fn remove_learner(&mut self, id: &NodeId) { self.learners.remove(id); } diff --git a/openraft/src/membership/membership_test.rs b/openraft/src/membership/membership_test.rs index e7693b15f..ec13e8184 100644 --- a/openraft/src/membership/membership_test.rs +++ b/openraft/src/membership/membership_test.rs @@ -34,6 +34,8 @@ fn test_membership() -> anyhow::Result<()> { assert!(m123_345.is_member(&4)); assert!(!m123_345.is_member(&6)); + assert_eq!(m1.is_turn_to_learner(), true); + assert!(!m123.is_in_joint_consensus()); assert!(m123_345.is_in_joint_consensus()); diff --git a/openraft/src/raft.rs b/openraft/src/raft.rs index be458bb86..5b3ac189d 100644 --- a/openraft/src/raft.rs +++ b/openraft/src/raft.rs @@ -251,6 +251,9 @@ impl, S: RaftStorage> Ra /// If blocking is true, it blocks until every learner becomes up to date. /// Otherwise it returns error `ChangeMembershipError::LearnerIsLagging` if there is a lagging learner. /// + /// If turn_to_learner is true, then all the members which not exists in the new membership, + /// will be turned into learners, otherwise will be removed. + /// /// If it lost leadership or crashed before committing the second **uniform** config log, the cluster is left in the /// **joint** config. #[tracing::instrument(level = "debug", skip(self))] @@ -258,6 +261,7 @@ impl, S: RaftStorage> Ra &self, members: BTreeSet, blocking: bool, + turn_to_learner: bool, ) -> Result, ClientWriteError> { tracing::info!("change_membership: start to commit joint config"); @@ -269,6 +273,7 @@ impl, S: RaftStorage> Ra RaftMsg::ChangeMembership { members: members.clone(), blocking, + turn_to_learner, tx, }, rx, @@ -288,7 +293,17 @@ impl, S: RaftStorage> Ra tracing::debug!("the second step is to change to uniform config: {:?}", members); let (tx, rx) = oneshot::channel(); - let res = self.call_core(RaftMsg::ChangeMembership { members, blocking, tx }, rx).await?; + let res = self + .call_core( + RaftMsg::ChangeMembership { + members, + blocking, + turn_to_learner, + tx, + }, + rx, + ) + .await?; tracing::info!("res of second change_membership: {}", res.summary()); @@ -445,6 +460,11 @@ pub(crate) enum RaftMsg { /// /// Otherwise, wait for commit of the member change log. blocking: bool, + + /// If turn_to_learner is true, then all the members which not exists in the new membership, + /// will be turned into learners, otherwise will be removed. + turn_to_learner: bool, + tx: RaftRespTx, ClientWriteError>, }, } @@ -475,8 +495,16 @@ where RaftMsg::AddLearner { id, blocking, .. } => { format!("AddLearner: id: {}, blocking: {}", id, blocking) } - RaftMsg::ChangeMembership { members, blocking, .. } => { - format!("ChangeMembership: members: {:?}, blocking: {}", members, blocking) + RaftMsg::ChangeMembership { + members, + blocking, + turn_to_learner, + .. + } => { + format!( + "ChangeMembership: members: {:?}, blocking: {}, turn_to_learner: {}", + members, blocking, turn_to_learner, + ) } } } diff --git a/openraft/tests/fixtures/mod.rs b/openraft/tests/fixtures/mod.rs index 0f6a448a2..e4861eda4 100644 --- a/openraft/tests/fixtures/mod.rs +++ b/openraft/tests/fixtures/mod.rs @@ -475,7 +475,18 @@ impl RaftRouter { ) -> Result, ClientWriteError> { let rt = self.routing_table.read().await; let node = rt.get(&leader).unwrap_or_else(|| panic!("node with ID {} does not exist", leader)); - node.0.change_membership(members, true).await + node.0.change_membership(members, true, true).await + } + + pub async fn change_membership_with_turn_to_learner( + &self, + leader: NodeId, + members: BTreeSet, + turn_to_learner: bool, + ) -> Result, ClientWriteError> { + let rt = self.routing_table.read().await; + let node = rt.get(&leader).unwrap_or_else(|| panic!("node with ID {} does not exist", leader)); + node.0.change_membership(members, true, turn_to_learner).await } pub async fn change_membership_with_blocking( @@ -486,7 +497,7 @@ impl RaftRouter { ) -> Result, ClientWriteError> { let rt = self.routing_table.read().await; let node = rt.get(&leader).unwrap_or_else(|| panic!("node with ID {} does not exist", leader)); - node.0.change_membership(members, blocking).await + node.0.change_membership(members, blocking, true).await } /// Send a client read request to the target node. diff --git a/openraft/tests/membership/main.rs b/openraft/tests/membership/main.rs index 4dbc83c59..464dca988 100644 --- a/openraft/tests/membership/main.rs +++ b/openraft/tests/membership/main.rs @@ -2,6 +2,7 @@ #[path = "../fixtures/mod.rs"] mod fixtures; +/* // The number indicate the preferred running order for these case. // The later tests may depend on the earlier ones. mod t00_learner_restart; @@ -18,3 +19,6 @@ mod t30_commit_joint_config; mod t30_step_down; mod t40_removed_follower; mod t99_new_leader_auto_commit_uniform_config; +*/ + +mod t20_change_membership; diff --git a/openraft/tests/membership/t20_change_membership.rs b/openraft/tests/membership/t20_change_membership.rs index 6991a687c..9503728e8 100644 --- a/openraft/tests/membership/t20_change_membership.rs +++ b/openraft/tests/membership/t20_change_membership.rs @@ -6,6 +6,7 @@ use maplit::btreeset; use openraft::error::ChangeMembershipError; use openraft::Config; use openraft::RaftStorage; +use openraft::State; use crate::fixtures::RaftRouter; @@ -119,6 +120,58 @@ async fn change_with_lagging_learner_non_blocking() -> anyhow::Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn change_with_remove_not_exist_member() -> anyhow::Result<()> { + // Add a member without adding it as learner, in blocking mode it should finish successfully. + + let (_log_guard, ut_span) = init_ut!(); + let _ent = ut_span.enter(); + + let config = Arc::new(Config { ..Default::default() }.validate()?); + let router = Arc::new(RaftRouter::new(config.clone())); + let timeout = Some(Duration::from_millis(1000)); + let mut n_logs = router.new_nodes_from_single(btreeset! {0,1,2}, btreeset! {}).await?; + + tracing::info!("--- write up to 1 logs"); + { + router.client_request_many(0, "non_voter_add", 1).await; + n_logs += 1; + + // all the nodes MUST recv the log + router.wait_for_log(&btreeset![0, 1, 2], Some(n_logs), timeout, "append a log").await?; + } + + { + router.change_membership_with_turn_to_learner(0, btreeset![0, 1], false).await?; + // 2 for change_membership + n_logs += 2; + + // all the nodes MUST recv the change_membership log + router.wait_for_log(&btreeset![0, 1], Some(n_logs), timeout, "append a log").await?; + } + + tracing::info!("--- write up to 1 logs"); + { + router.client_request_many(0, "non_voter_add", 1).await; + n_logs += 1; + + // node [0,1] MUST recv the log + router.wait_for_log(&btreeset![0, 1], Some(n_logs), timeout, "append a log").await?; + + // node 2 MUST be shutdown + router + .wait_for_metrics( + &2, + |x| x.state == State::Shutdown, + timeout, + &format!("n{}.state -> {:?}", 2, State::Shutdown), + ) + .await?; + } + + Ok(()) +} + fn timeout() -> Option { Some(Duration::from_micros(500)) } From 243b97b3e3db43fa29b4976dbb0b0708df49546f Mon Sep 17 00:00:00 2001 From: lichuang Date: Tue, 25 Jan 2022 11:44:03 +0800 Subject: [PATCH 02/10] Refactor: add option in function change_membership, turn not exist member into learner, or remove it --- openraft/tests/membership/main.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/openraft/tests/membership/main.rs b/openraft/tests/membership/main.rs index 464dca988..4dbc83c59 100644 --- a/openraft/tests/membership/main.rs +++ b/openraft/tests/membership/main.rs @@ -2,7 +2,6 @@ #[path = "../fixtures/mod.rs"] mod fixtures; -/* // The number indicate the preferred running order for these case. // The later tests may depend on the earlier ones. mod t00_learner_restart; @@ -19,6 +18,3 @@ mod t30_commit_joint_config; mod t30_step_down; mod t40_removed_follower; mod t99_new_leader_auto_commit_uniform_config; -*/ - -mod t20_change_membership; From 4374b4f28390222c0ecde5038fdf3eeb1be0a98e Mon Sep 17 00:00:00 2001 From: lichuang Date: Tue, 25 Jan 2022 12:22:06 +0800 Subject: [PATCH 03/10] Fix: make clippy happy --- openraft/src/core/mod.rs | 12 +++++------- openraft/src/membership/membership.rs | 2 +- openraft/src/membership/membership_test.rs | 2 +- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/openraft/src/core/mod.rs b/openraft/src/core/mod.rs index 2d8dfd39f..287adec7f 100644 --- a/openraft/src/core/mod.rs +++ b/openraft/src/core/mod.rs @@ -449,14 +449,12 @@ impl, S: RaftStorage> Ra // Transition to follower. self.set_target_state(State::Follower); } + } else if self.effective_membership.membership.is_turn_to_learner() { + self.set_target_state(State::Learner); + tracing::debug!("node {} turn to learner", self.id); } else { - if self.effective_membership.membership.is_turn_to_learner() { - self.set_target_state(State::Learner); - tracing::debug!("node {} turn to learner", self.id); - } else { - self.set_target_state(State::Shutdown); - tracing::debug!("node {} has been removed,shutdowning...", self.id); - } + self.set_target_state(State::Shutdown); + tracing::debug!("node {} has been removed,shutdowning...", self.id); } } diff --git a/openraft/src/membership/membership.rs b/openraft/src/membership/membership.rs index ba28d9214..f53dcbf5a 100644 --- a/openraft/src/membership/membership.rs +++ b/openraft/src/membership/membership.rs @@ -110,7 +110,7 @@ impl Membership { learners.insert(*id); let configs = self.configs.clone(); let all_members = Self::build_all_members(&self.configs); - let turn_to_learner = self.turn_to_learner.clone(); + let turn_to_learner = self.turn_to_learner; Membership { learners, configs, diff --git a/openraft/src/membership/membership_test.rs b/openraft/src/membership/membership_test.rs index ec13e8184..0c07ec1ec 100644 --- a/openraft/src/membership/membership_test.rs +++ b/openraft/src/membership/membership_test.rs @@ -34,7 +34,7 @@ fn test_membership() -> anyhow::Result<()> { assert!(m123_345.is_member(&4)); assert!(!m123_345.is_member(&6)); - assert_eq!(m1.is_turn_to_learner(), true); + assert!(m1.is_turn_to_learner()); assert!(!m123.is_in_joint_consensus()); assert!(m123_345.is_in_joint_consensus()); From 36dd7a23d2bff169da982cc03b77bbc11f091412 Mon Sep 17 00:00:00 2001 From: lichuang Date: Tue, 25 Jan 2022 14:37:10 +0800 Subject: [PATCH 04/10] Fix: rerun ci From 6713c388185569c5060e0360ac898af163026b32 Mon Sep 17 00:00:00 2001 From: lichuang Date: Wed, 9 Feb 2022 18:34:47 +0800 Subject: [PATCH 05/10] Refactor: refactor turn_into_learner logic --- openraft/src/core/admin.rs | 14 +++++++-- openraft/src/core/mod.rs | 6 +--- openraft/src/membership/membership.rs | 30 ++++++------------- openraft/src/membership/membership_test.rs | 6 ++-- openraft/tests/fixtures/mod.rs | 4 +-- .../tests/membership/t20_change_membership.rs | 8 ++--- 6 files changed, 32 insertions(+), 36 deletions(-) diff --git a/openraft/src/core/admin.rs b/openraft/src/core/admin.rs index f43233e47..4f57363b2 100644 --- a/openraft/src/core/admin.rs +++ b/openraft/src/core/admin.rs @@ -166,8 +166,18 @@ impl<'a, D: AppData, R: AppDataResponse, N: RaftNetwork, S: RaftStorage let curr = self.core.effective_membership.membership.clone(); let new_members = members.difference(curr.all_members()); - let mut new_config = curr.next_safe(members.clone()); - new_config.set_turn_to_learner(turn_to_learner); + let mut new_config = if turn_to_learner { + // add removed members into learners + let removed_members = curr.all_members().difference(&members); + let mut learners = curr.all_learners().clone(); + for id in removed_members { + learners.insert(*id); + } + curr.next_safe_with_learners(members.clone(), learners) + } else { + curr.next_safe(members.clone()) + }; + tracing::debug!(?new_config, "new_config"); // Check the proposed config for any new nodes. If ALL new nodes already have replication diff --git a/openraft/src/core/mod.rs b/openraft/src/core/mod.rs index 1fcbbcb2e..093f100af 100644 --- a/openraft/src/core/mod.rs +++ b/openraft/src/core/mod.rs @@ -422,12 +422,8 @@ impl, S: RaftStorage> Ra // Transition to follower. self.set_target_state(State::Follower); } - } else if self.effective_membership.membership.is_turn_to_learner() { - self.set_target_state(State::Learner); - tracing::debug!("node {} turn to learner", self.id); } else { - self.set_target_state(State::Shutdown); - tracing::debug!("node {} has been removed,shutdowning...", self.id); + self.set_target_state(State::Learner); } } diff --git a/openraft/src/membership/membership.rs b/openraft/src/membership/membership.rs index f53dcbf5a..57764115f 100644 --- a/openraft/src/membership/membership.rs +++ b/openraft/src/membership/membership.rs @@ -27,9 +27,6 @@ pub struct Membership { /// Cache of all node ids. all_members: BTreeSet, - - /// default: true - turn_to_learner: bool, } impl MessageSummary for Membership { @@ -58,49 +55,41 @@ impl MessageSummary for Membership { impl Membership { pub fn new_single(members: BTreeSet) -> Self { let configs = vec![members]; - let turn_to_learner = true; let all_members = Self::build_all_members(&configs); let learners = BTreeSet::new(); Membership { learners, configs, all_members, - turn_to_learner, } } pub fn new_single_with_learners(members: BTreeSet, learners: BTreeSet) -> Self { let configs = vec![members]; let all_members = Self::build_all_members(&configs); - let turn_to_learner = true; Membership { learners, configs, all_members, - turn_to_learner, } } pub fn new_multi(configs: Vec>) -> Self { let all_members = Self::build_all_members(&configs); let learners = BTreeSet::new(); - let turn_to_learner = true; Membership { learners, configs, all_members, - turn_to_learner, } } pub fn new_multi_with_learners(configs: Vec>, learners: BTreeSet) -> Self { let all_members = Self::build_all_members(&configs); - let turn_to_learner = true; Membership { learners, configs, all_members, - turn_to_learner, } } @@ -110,23 +99,13 @@ impl Membership { learners.insert(*id); let configs = self.configs.clone(); let all_members = Self::build_all_members(&self.configs); - let turn_to_learner = self.turn_to_learner; Membership { learners, configs, all_members, - turn_to_learner, } } - pub fn set_turn_to_learner(&mut self, turn_to_learner: bool) { - self.turn_to_learner = turn_to_learner; - } - - pub fn is_turn_to_learner(&self) -> bool { - self.turn_to_learner - } - pub fn remove_learner(&mut self, id: &NodeId) { self.learners.remove(id); } @@ -291,6 +270,15 @@ impl Membership { } } + #[must_use] + pub fn next_safe_with_learners(&self, goal: BTreeSet, learners: BTreeSet) -> Self { + if self.configs.contains(&goal) { + Membership::new_single_with_learners(goal, learners) + } else { + Membership::new_multi_with_learners(vec![self.configs.last().cloned().unwrap(), goal], learners) + } + } + fn is_majority_of_single_config(granted: &BTreeSet, single_config: &BTreeSet) -> bool { let d = granted.intersection(single_config); let n_granted = d.fold(0, |a, _x| a + 1); diff --git a/openraft/src/membership/membership_test.rs b/openraft/src/membership/membership_test.rs index 0c07ec1ec..0b7d23173 100644 --- a/openraft/src/membership/membership_test.rs +++ b/openraft/src/membership/membership_test.rs @@ -34,8 +34,6 @@ fn test_membership() -> anyhow::Result<()> { assert!(m123_345.is_member(&4)); assert!(!m123_345.is_member(&6)); - assert!(m1.is_turn_to_learner()); - assert!(!m123.is_in_joint_consensus()); assert!(m123_345.is_in_joint_consensus()); @@ -254,5 +252,9 @@ fn test_membership_next_safe() -> anyhow::Result<()> { assert_eq!(m2, m12.next_safe(c2())); assert_eq!(m23, m12.next_safe(c3())); + let learners = || btreeset! {10,20,30}; + let m23_with_learners = Membership::new_multi_with_learners(vec![c2(), c3()], learners()); + assert_eq!(m23_with_learners, m12.next_safe_with_learners(c3(), learners())); + Ok(()) } diff --git a/openraft/tests/fixtures/mod.rs b/openraft/tests/fixtures/mod.rs index bcdd0e24b..21bfd387e 100644 --- a/openraft/tests/fixtures/mod.rs +++ b/openraft/tests/fixtures/mod.rs @@ -476,7 +476,7 @@ impl RaftRouter { ) -> Result, ClientWriteError> { let rt = self.routing_table.read().await; let node = rt.get(&leader).unwrap_or_else(|| panic!("node with ID {} does not exist", leader)); - node.0.change_membership(members, true, true).await + node.0.change_membership(members, true, false).await } pub async fn change_membership_with_turn_to_learner( @@ -498,7 +498,7 @@ impl RaftRouter { ) -> Result, ClientWriteError> { let rt = self.routing_table.read().await; let node = rt.get(&leader).unwrap_or_else(|| panic!("node with ID {} does not exist", leader)); - node.0.change_membership(members, blocking, true).await + node.0.change_membership(members, blocking, false).await } /// Send a client read request to the target node. diff --git a/openraft/tests/membership/t20_change_membership.rs b/openraft/tests/membership/t20_change_membership.rs index 9503728e8..5ba94bc3b 100644 --- a/openraft/tests/membership/t20_change_membership.rs +++ b/openraft/tests/membership/t20_change_membership.rs @@ -121,7 +121,7 @@ async fn change_with_lagging_learner_non_blocking() -> anyhow::Result<()> { } #[tokio::test(flavor = "multi_thread", worker_threads = 4)] -async fn change_with_remove_not_exist_member() -> anyhow::Result<()> { +async fn change_with_turn_not_exist_member_to_learner() -> anyhow::Result<()> { // Add a member without adding it as learner, in blocking mode it should finish successfully. let (_log_guard, ut_span) = init_ut!(); @@ -142,7 +142,7 @@ async fn change_with_remove_not_exist_member() -> anyhow::Result<()> { } { - router.change_membership_with_turn_to_learner(0, btreeset![0, 1], false).await?; + router.change_membership_with_turn_to_learner(0, btreeset![0, 1], true).await?; // 2 for change_membership n_logs += 2; @@ -162,9 +162,9 @@ async fn change_with_remove_not_exist_member() -> anyhow::Result<()> { router .wait_for_metrics( &2, - |x| x.state == State::Shutdown, + |x| x.state == State::Learner, timeout, - &format!("n{}.state -> {:?}", 2, State::Shutdown), + &format!("n{}.state -> {:?}", 2, State::Learner), ) .await?; } From 742581cc9c0e7bd621a707c268602d2eea30cd63 Mon Sep 17 00:00:00 2001 From: lichuang Date: Wed, 9 Feb 2022 19:19:59 +0800 Subject: [PATCH 06/10] Refactor: add turn_into_learner param into next_safe function --- openraft/src/core/admin.rs | 12 +---------- openraft/src/membership/membership.rs | 24 +++++++++++----------- openraft/src/membership/membership_test.rs | 12 +++++------ openraft/src/raft.rs | 4 ++-- 4 files changed, 21 insertions(+), 31 deletions(-) diff --git a/openraft/src/core/admin.rs b/openraft/src/core/admin.rs index 4f57363b2..49020a653 100644 --- a/openraft/src/core/admin.rs +++ b/openraft/src/core/admin.rs @@ -166,17 +166,7 @@ impl<'a, D: AppData, R: AppDataResponse, N: RaftNetwork, S: RaftStorage let curr = self.core.effective_membership.membership.clone(); let new_members = members.difference(curr.all_members()); - let mut new_config = if turn_to_learner { - // add removed members into learners - let removed_members = curr.all_members().difference(&members); - let mut learners = curr.all_learners().clone(); - for id in removed_members { - learners.insert(*id); - } - curr.next_safe_with_learners(members.clone(), learners) - } else { - curr.next_safe(members.clone()) - }; + let mut new_config = curr.next_safe(members.clone(), turn_to_learner); tracing::debug!(?new_config, "new_config"); diff --git a/openraft/src/membership/membership.rs b/openraft/src/membership/membership.rs index 57764115f..dd89c0d6f 100644 --- a/openraft/src/membership/membership.rs +++ b/openraft/src/membership/membership.rs @@ -259,19 +259,19 @@ impl Membership { /// } /// ``` #[must_use] - pub fn next_safe(&self, goal: BTreeSet) -> Self { - if self.configs.contains(&goal) { - Membership::new_single_with_learners(goal, self.learners.clone()) + pub fn next_safe(&self, goal: BTreeSet, turn_to_learner: bool) -> Self { + let learners = if turn_to_learner { + let curr = self.clone(); + // add removed members into learners + let removed_members = curr.all_members().difference(&goal); + let mut learners = curr.all_learners().clone(); + for id in removed_members { + learners.insert(*id); + } + learners } else { - Membership::new_multi_with_learners( - vec![self.configs.last().cloned().unwrap(), goal], - self.learners.clone(), - ) - } - } - - #[must_use] - pub fn next_safe_with_learners(&self, goal: BTreeSet, learners: BTreeSet) -> Self { + self.learners.clone() + }; if self.configs.contains(&goal) { Membership::new_single_with_learners(goal, learners) } else { diff --git a/openraft/src/membership/membership_test.rs b/openraft/src/membership/membership_test.rs index 0b7d23173..79c743043 100644 --- a/openraft/src/membership/membership_test.rs +++ b/openraft/src/membership/membership_test.rs @@ -246,15 +246,15 @@ fn test_membership_next_safe() -> anyhow::Result<()> { let m12 = Membership::new_multi(vec![c1(), c2()]); let m23 = Membership::new_multi(vec![c2(), c3()]); - assert_eq!(m1, m1.next_safe(c1())); - assert_eq!(m12, m1.next_safe(c2())); - assert_eq!(m1, m12.next_safe(c1())); - assert_eq!(m2, m12.next_safe(c2())); - assert_eq!(m23, m12.next_safe(c3())); + assert_eq!(m1, m1.next_safe(c1(), false)); + assert_eq!(m12, m1.next_safe(c2(), false)); + assert_eq!(m1, m12.next_safe(c1(), false)); + assert_eq!(m2, m12.next_safe(c2(), false)); + assert_eq!(m23, m12.next_safe(c3(), false)); let learners = || btreeset! {10,20,30}; let m23_with_learners = Membership::new_multi_with_learners(vec![c2(), c3()], learners()); - assert_eq!(m23_with_learners, m12.next_safe_with_learners(c3(), learners())); + assert_eq!(m23_with_learners, m12.next_safe(c3(), true)); Ok(()) } diff --git a/openraft/src/raft.rs b/openraft/src/raft.rs index ce0d57e60..3ccffdbf5 100644 --- a/openraft/src/raft.rs +++ b/openraft/src/raft.rs @@ -249,10 +249,10 @@ impl, S: RaftStorage> Ra /// - It proposes a **joint** config. /// - When the **joint** config is committed, it proposes a uniform config. /// - /// If blocking is true, it blocks until every learner becomes up to date. + /// If `blocking` is true, it blocks until every learner becomes up to date. /// Otherwise it returns error `ChangeMembershipError::LearnerIsLagging` if there is a lagging learner. /// - /// If turn_to_learner is true, then all the members which not exists in the new membership, + /// If `turn_to_learner` is true, then all the members which not exists in the new membership, /// will be turned into learners, otherwise will be removed. /// /// If it lost leadership or crashed before committing the second **uniform** config log, the cluster is left in the From 433fa6299d37c202aa3f91b83dee1f4052feecd7 Mon Sep 17 00:00:00 2001 From: lichuang Date: Wed, 9 Feb 2022 19:43:15 +0800 Subject: [PATCH 07/10] Refactor: add turn_into_learner param into next_safe function, fix test error --- openraft/src/membership/membership_test.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/openraft/src/membership/membership_test.rs b/openraft/src/membership/membership_test.rs index 79c743043..0c1b9b87b 100644 --- a/openraft/src/membership/membership_test.rs +++ b/openraft/src/membership/membership_test.rs @@ -252,9 +252,11 @@ fn test_membership_next_safe() -> anyhow::Result<()> { assert_eq!(m2, m12.next_safe(c2(), false)); assert_eq!(m23, m12.next_safe(c3(), false)); - let learners = || btreeset! {10,20,30}; - let m23_with_learners = Membership::new_multi_with_learners(vec![c2(), c3()], learners()); - assert_eq!(m23_with_learners, m12.next_safe(c3(), true)); + let old_learners = || btreeset! {1, 2}; + let learners = || btreeset! {1, 2, 3, 4, 5}; + let m23_with_learners_old = Membership::new_multi_with_learners(vec![c2(), c3()], old_learners()); + let m23_with_learners_new = Membership::new_multi_with_learners(vec![c3()], learners()); + assert_eq!(m23_with_learners_new, m23_with_learners_old.next_safe(c3(), true)); Ok(()) } From b288e52cd944c600a9467945cdf36e7966b337eb Mon Sep 17 00:00:00 2001 From: lichuang Date: Wed, 9 Feb 2022 19:57:04 +0800 Subject: [PATCH 08/10] Refactor: change comment --- openraft/tests/membership/t20_change_membership.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openraft/tests/membership/t20_change_membership.rs b/openraft/tests/membership/t20_change_membership.rs index 5ba94bc3b..5e2565e5d 100644 --- a/openraft/tests/membership/t20_change_membership.rs +++ b/openraft/tests/membership/t20_change_membership.rs @@ -158,7 +158,7 @@ async fn change_with_turn_not_exist_member_to_learner() -> anyhow::Result<()> { // node [0,1] MUST recv the log router.wait_for_log(&btreeset![0, 1], Some(n_logs), timeout, "append a log").await?; - // node 2 MUST be shutdown + // node 2 MUST stay in learner state and is able to receive new logs router .wait_for_metrics( &2, From 89066e4b166080dd48a910f289649a87183aa887 Mon Sep 17 00:00:00 2001 From: lichuang Date: Wed, 9 Feb 2022 21:59:46 +0800 Subject: [PATCH 09/10] Refactor: add more tests --- openraft/tests/fixtures/mod.rs | 19 +++++++++++++++++++ .../tests/membership/t20_change_membership.rs | 6 ++++++ 2 files changed, 25 insertions(+) diff --git a/openraft/tests/fixtures/mod.rs b/openraft/tests/fixtures/mod.rs index 21bfd387e..dd3fbf66a 100644 --- a/openraft/tests/fixtures/mod.rs +++ b/openraft/tests/fixtures/mod.rs @@ -399,6 +399,25 @@ impl RaftRouter { Ok(()) } + #[tracing::instrument(level = "info", skip(self))] + pub async fn wait_for_members( + &self, + node_ids: &BTreeSet, + members: BTreeSet, + timeout: Option, + msg: &str, + ) -> Result<()> { + for i in node_ids.iter() { + let wait = self.wait(i, timeout).await?; + wait.metrics( + |x| x.membership_config.membership.get_ith_config(0).cloned().unwrap() == members, + msg, + ) + .await?; + } + Ok(()) + } + /// Wait for specified nodes until their state becomes `state`. #[tracing::instrument(level = "info", skip(self))] pub async fn wait_for_state( diff --git a/openraft/tests/membership/t20_change_membership.rs b/openraft/tests/membership/t20_change_membership.rs index 5e2565e5d..b9f4573d0 100644 --- a/openraft/tests/membership/t20_change_membership.rs +++ b/openraft/tests/membership/t20_change_membership.rs @@ -167,6 +167,12 @@ async fn change_with_turn_not_exist_member_to_learner() -> anyhow::Result<()> { &format!("n{}.state -> {:?}", 2, State::Learner), ) .await?; + + // node [2] MUST recv the log + router.wait_for_log(&btreeset![2], Some(n_logs), timeout, "append a log").await?; + + // check membership + router.wait_for_members(&btreeset![0, 1, 2], btreeset![0, 1], timeout, "members: [0,1]").await?; } Ok(()) From a5486518b83ebe9a5fecc0a6cf376eba58fb6be3 Mon Sep 17 00:00:00 2001 From: lichuang Date: Thu, 10 Feb 2022 14:44:56 +0800 Subject: [PATCH 10/10] Refactor: add more tests --- openraft/tests/fixtures/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/openraft/tests/fixtures/mod.rs b/openraft/tests/fixtures/mod.rs index dd3fbf66a..b01057779 100644 --- a/openraft/tests/fixtures/mod.rs +++ b/openraft/tests/fixtures/mod.rs @@ -410,7 +410,10 @@ impl RaftRouter { for i in node_ids.iter() { let wait = self.wait(i, timeout).await?; wait.metrics( - |x| x.membership_config.membership.get_ith_config(0).cloned().unwrap() == members, + |x| { + x.membership_config.membership.get_configs().len() == 1 + && x.membership_config.membership.get_ith_config(0).cloned().unwrap() == members + }, msg, ) .await?;