From 67375a2a44cb06a84815df1119c28dcf2681f842 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BC=A0=E7=82=8E=E6=B3=BC?= Date: Tue, 5 Apr 2022 13:50:27 +0800 Subject: [PATCH] Change: RaftStorage: use `EffectiveMembership` instead of `Option<_>` The value of membership config of an uninitialized raft-node should be: ```rust EffectiveMembership { log_id: None, membership: Membership::default(), } ``` I.e., there is always a membership config for either an initialized or an uninitialized raft-node. Because `EffectiveMembership` is not a value that could be `None`, but it is more like a container of values. Changed struct: ```rust struct InitialState { pub last_membership: EffectiveMembership, ... } ``` Changed methods: ```rust RaftStorage::get_membership() -> Result, _>; RaftStorage::last_applied_state() -> Result<(_, EffectiveMembership), _>; ``` --- example-raft-kv/src/store/mod.rs | 13 ++++------- memstore/src/lib.rs | 6 ++--- openraft/src/core/append_entries.rs | 3 --- openraft/src/core/install_snapshot.rs | 4 ---- openraft/src/core/mod.rs | 2 +- openraft/src/storage.rs | 15 +++++------- openraft/src/store_ext.rs | 2 +- openraft/src/testing/suite.rs | 23 ++++++++----------- openraft/tests/initialization.rs | 4 ++-- openraft/tests/membership/t10_add_learner.rs | 1 - .../snapshot/snapshot_overrides_membership.rs | 4 ---- .../snapshot_uses_prev_snap_membership.rs | 4 ---- .../t20_state_machine_apply_membership.rs | 8 +++---- 13 files changed, 30 insertions(+), 59 deletions(-) diff --git a/example-raft-kv/src/store/mod.rs b/example-raft-kv/src/store/mod.rs index 5d4dacdfe..71cddbf7d 100644 --- a/example-raft-kv/src/store/mod.rs +++ b/example-raft-kv/src/store/mod.rs @@ -73,7 +73,7 @@ pub struct ExampleStateMachine { pub last_applied_log: Option>, // TODO: it should not be Option. - pub last_membership: Option>, + pub last_membership: EffectiveMembership, /// Application data. pub data: BTreeMap, @@ -252,13 +252,8 @@ impl RaftStorage for Arc { async fn last_applied_state( &mut self, - ) -> Result< - ( - Option>, - Option>, - ), - StorageError, - > { + ) -> Result<(Option>, EffectiveMembership), StorageError> + { let state_machine = self.state_machine.read().await; Ok((state_machine.last_applied_log, state_machine.last_membership.clone())) } @@ -288,7 +283,7 @@ impl RaftStorage for Arc { } }, EntryPayload::Membership(ref mem) => { - sm.last_membership = Some(EffectiveMembership::new(Some(entry.log_id), mem.clone())); + sm.last_membership = EffectiveMembership::new(Some(entry.log_id), mem.clone()); res.push(ExampleResponse { value: None }) } }; diff --git a/memstore/src/lib.rs b/memstore/src/lib.rs index c544d9cba..add8bde11 100644 --- a/memstore/src/lib.rs +++ b/memstore/src/lib.rs @@ -90,7 +90,7 @@ pub struct MemStoreSnapshot { pub struct MemStoreStateMachine { pub last_applied_log: Option>, - pub last_membership: Option>, + pub last_membership: EffectiveMembership, /// A mapping of client IDs to their state info. pub client_serial_responses: HashMap)>, @@ -263,7 +263,7 @@ impl RaftStorage for Arc { async fn last_applied_state( &mut self, - ) -> Result<(Option>, Option>), StorageError> { + ) -> Result<(Option>, EffectiveMembership), StorageError> { let sm = self.sm.read().await; Ok((sm.last_applied_log, sm.last_membership.clone())) } @@ -343,7 +343,7 @@ impl RaftStorage for Arc { res.push(ClientResponse(previous)); } EntryPayload::Membership(ref mem) => { - sm.last_membership = Some(EffectiveMembership::new(Some(entry.log_id), mem.clone())); + sm.last_membership = EffectiveMembership::new(Some(entry.log_id), mem.clone()); res.push(ClientResponse(None)) } }; diff --git a/openraft/src/core/append_entries.rs b/openraft/src/core/append_entries.rs index 7bbfa0de0..891a5e0c8 100644 --- a/openraft/src/core/append_entries.rs +++ b/openraft/src/core/append_entries.rs @@ -77,9 +77,6 @@ impl, S: RaftStorage> RaftCore, S: RaftStorage> RaftCore, S: RaftStorage> RaftCore { /// The latest cluster membership configuration found, in log or in state machine, else a new initial /// membership config consisting only of this node's ID. - pub last_membership: Option>, + pub last_membership: EffectiveMembership, } /// The state about logs. @@ -193,18 +193,15 @@ where C: RaftTypeConfig /// TODO(xp): if there is no membership log, return `{log_id:None, membership: vec![]}`. /// The raft core should always have an effective_membership. /// Initially, it is empty. - async fn get_membership(&mut self) -> Result>, StorageError> { + async fn get_membership(&mut self) -> Result, StorageError> { let (_, sm_mem) = self.last_applied_state().await?; - let sm_mem_next_index = match &sm_mem { - None => 0, - Some(mem) => mem.log_id.next_index(), - }; + let sm_mem_next_index = sm_mem.log_id.next_index(); let log_mem = self.last_membership_in_log(sm_mem_next_index).await?; - if log_mem.is_some() { - return Ok(log_mem); + if let Some(x) = log_mem { + return Ok(x); } return Ok(sm_mem); @@ -311,7 +308,7 @@ where C: RaftTypeConfig // NOTE: This can be made into sync, provided all state machines will use atomic read or the like. async fn last_applied_state( &mut self, - ) -> Result<(Option>, Option>), StorageError>; + ) -> Result<(Option>, EffectiveMembership), StorageError>; /// Apply the given payload of entries to the state machine. /// diff --git a/openraft/src/store_ext.rs b/openraft/src/store_ext.rs index e2d5e7ecb..011bc5345 100644 --- a/openraft/src/store_ext.rs +++ b/openraft/src/store_ext.rs @@ -124,7 +124,7 @@ where #[tracing::instrument(level = "trace", skip(self))] async fn last_applied_state( &mut self, - ) -> Result<(Option>, Option>), StorageError> { + ) -> Result<(Option>, EffectiveMembership), StorageError> { self.inner().last_applied_state().await } diff --git a/openraft/src/testing/suite.rs b/openraft/src/testing/suite.rs index 59374a9a2..31c8427d7 100644 --- a/openraft/src/testing/suite.rs +++ b/openraft/src/testing/suite.rs @@ -194,7 +194,7 @@ where let membership = store.get_membership().await?; - assert!(membership.is_none()); + assert_eq!(EffectiveMembership::default(), membership); Ok(()) } @@ -218,7 +218,6 @@ where .await?; let mem = store.get_membership().await?; - let mem = mem.unwrap(); assert_eq!(Membership::new(vec![btreeset! {3,4,5}], None), mem.membership,); } @@ -234,8 +233,6 @@ where let mem = store.get_membership().await?; - let mem = mem.unwrap(); - assert_eq!(Membership::new(vec![btreeset! {3, 4, 5}], None), mem.membership,); } @@ -250,8 +247,6 @@ where let mem = store.get_membership().await?; - let mem = mem.unwrap(); - assert_eq!(Membership::new(vec![btreeset! {7,8,9}], None), mem.membership,); } @@ -335,7 +330,7 @@ where assert_eq!( Membership::new(vec![btreeset! {3,4,5}], None), - initial.last_membership.unwrap().membership, + initial.last_membership.membership, ); } @@ -352,7 +347,7 @@ where assert_eq!( Membership::new(vec![btreeset! {3,4,5}], None), - initial.last_membership.unwrap().membership, + initial.last_membership.membership, ); } @@ -369,7 +364,7 @@ where assert_eq!( Membership::new(vec![btreeset! {1,2,3}], None), - initial.last_membership.unwrap().membership, + initial.last_membership.membership, ); } @@ -623,7 +618,7 @@ where let (applied, membership) = store.last_applied_state().await?; assert_eq!(None, applied); - assert_eq!(None, membership); + assert_eq!(EffectiveMembership::default(), membership); tracing::info!("--- with last_applied and last_membership"); { @@ -637,10 +632,10 @@ where let (applied, membership) = store.last_applied_state().await?; assert_eq!(Some(LogId::new(LeaderId::new(1, NODE_ID.into()), 3)), applied); assert_eq!( - Some(EffectiveMembership::new( + EffectiveMembership::new( Some(LogId::new(LeaderId::new(1, NODE_ID.into()), 3)), Membership::new(vec![btreeset! {1,2}], None) - )), + ), membership ); } @@ -657,10 +652,10 @@ where let (applied, membership) = store.last_applied_state().await?; assert_eq!(Some(LogId::new(LeaderId::new(1, NODE_ID.into()), 5)), applied); assert_eq!( - Some(EffectiveMembership::new( + EffectiveMembership::new( Some(LogId::new(LeaderId::new(1, NODE_ID.into()), 3)), Membership::new(vec![btreeset! {1,2}], None) - )), + ), membership ); } diff --git a/openraft/tests/initialization.rs b/openraft/tests/initialization.rs index 8c88fd219..e7b5139da 100644 --- a/openraft/tests/initialization.rs +++ b/openraft/tests/initialization.rs @@ -87,10 +87,10 @@ async fn initialization() -> Result<()> { let sm_mem = sto.last_applied_state().await?.1; assert_eq!( - Some(EffectiveMembership::new( + EffectiveMembership::new( Some(LogId::new(LeaderId::new(0, 0), 0)), Membership::new(vec![btreeset! {0,1,2}], None) - )), + ), sm_mem ); } diff --git a/openraft/tests/membership/t10_add_learner.rs b/openraft/tests/membership/t10_add_learner.rs index 68bca9922..578b55ece 100644 --- a/openraft/tests/membership/t10_add_learner.rs +++ b/openraft/tests/membership/t10_add_learner.rs @@ -220,7 +220,6 @@ async fn check_learner_after_leader_transfered() -> Result<()> { { let mut sto = router.get_storage_handle(&1)?; let m = sto.get_membership().await?; - let m = m.unwrap(); assert_eq!( Membership::new(vec![btreeset! {1,3,4}], Some(btreeset! {2})), diff --git a/openraft/tests/snapshot/snapshot_overrides_membership.rs b/openraft/tests/snapshot/snapshot_overrides_membership.rs index 066716fb1..c0812868a 100644 --- a/openraft/tests/snapshot/snapshot_overrides_membership.rs +++ b/openraft/tests/snapshot/snapshot_overrides_membership.rs @@ -101,8 +101,6 @@ async fn snapshot_overrides_membership() -> Result<()> { { let m = sto.get_membership().await?; - let m = m.unwrap(); - assert_eq!(Membership::new(vec![btreeset! {2,3}], None), m.membership); } } @@ -133,8 +131,6 @@ async fn snapshot_overrides_membership() -> Result<()> { let m = sto.get_membership().await?; - let m = m.unwrap(); - assert_eq!( Membership::new(vec![btreeset! {0}], Some(btreeset! {1})), m.membership, diff --git a/openraft/tests/snapshot/snapshot_uses_prev_snap_membership.rs b/openraft/tests/snapshot/snapshot_uses_prev_snap_membership.rs index f901cd122..676941974 100644 --- a/openraft/tests/snapshot/snapshot_uses_prev_snap_membership.rs +++ b/openraft/tests/snapshot/snapshot_uses_prev_snap_membership.rs @@ -75,8 +75,6 @@ async fn snapshot_uses_prev_snap_membership() -> Result<()> { } let m = sto0.get_membership().await?; - let m = m.unwrap(); - assert_eq!( Membership::new(vec![btreeset! {0,1}], None), m.membership, @@ -125,8 +123,6 @@ async fn snapshot_uses_prev_snap_membership() -> Result<()> { } let m = sto0.get_membership().await?; - let m = m.unwrap(); - assert_eq!( Membership::new(vec![btreeset! {0,1}], None), m.membership, diff --git a/openraft/tests/state_machine/t20_state_machine_apply_membership.rs b/openraft/tests/state_machine/t20_state_machine_apply_membership.rs index 1bc146c87..e9ab86c23 100644 --- a/openraft/tests/state_machine/t20_state_machine_apply_membership.rs +++ b/openraft/tests/state_machine/t20_state_machine_apply_membership.rs @@ -49,10 +49,10 @@ async fn state_machine_apply_membership() -> Result<()> { for i in 0..=0 { let mut sto = router.get_storage_handle(&i)?; assert_eq!( - Some(EffectiveMembership::new( + EffectiveMembership::new( Some(LogId::new(LeaderId::new(0, 0), 0)), Membership::new(vec![btreeset! {0}], None) - )), + ), sto.last_applied_state().await?.1 ); } @@ -101,10 +101,10 @@ async fn state_machine_apply_membership() -> Result<()> { let mut sto = router.get_storage_handle(&i)?; let (_, last_membership) = sto.last_applied_state().await?; assert_eq!( - Some(EffectiveMembership::new( + EffectiveMembership::new( Some(LogId::new(LeaderId::new(1, 0), log_index)), Membership::new(vec![btreeset! {0, 1, 2}], Some(btreeset! {3,4})) - )), + ), last_membership ); }