Skip to content

Commit 964047b

Browse files
drmingdrmerdrmingdrmer
authored
Merge pull request #758 from drmingdrmer/43-refact
Chore: remove unused Command::UpdateMembership
2 parents 27aca2d + c68a7ee commit 964047b

14 files changed

+22
-132
lines changed

‎openraft/src/core/raft_core.rs

-3
Original file line numberDiff line numberDiff line change
@@ -1463,9 +1463,6 @@ impl<C: RaftTypeConfig, N: RaftNetworkFactory<C>, S: RaftStorage<C>> RaftRuntime
14631463
Command::UpdateProgressMetrics { target, matching } => {
14641464
self.update_progress_metrics(target, matching);
14651465
}
1466-
Command::UpdateMembership { .. } => {
1467-
// TODO: not used
1468-
}
14691466
Command::CancelSnapshot { snapshot_meta } => {
14701467
let got = self.received_snapshot.remove(&snapshot_meta.snapshot_id);
14711468
debug_assert!(got.is_some(), "there has to be a buffered snapshot data");

‎openraft/src/engine/command.rs

-15
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
use std::fmt::Debug;
2-
use std::sync::Arc;
32

43
use tokio::sync::oneshot;
54

@@ -13,7 +12,6 @@ use crate::raft::AppendEntriesResponse;
1312
use crate::raft::InstallSnapshotResponse;
1413
use crate::raft::VoteRequest;
1514
use crate::raft::VoteResponse;
16-
use crate::EffectiveMembership;
1715
use crate::LogId;
1816
use crate::MetricsChangeFlags;
1917
use crate::Node;
@@ -69,17 +67,6 @@ where
6967
/// Replicate log entries or snapshot to a target.
7068
Replicate { target: NID, req: Inflight<NID> },
7169

72-
// /// Replicate a snapshot to a target.
73-
// ReplicateSnapshot {
74-
// target: NID,
75-
// snapshot_last_log_id: Option<LogId<NID>>,
76-
// },
77-
/// Membership config changed, need to update replication streams.
78-
UpdateMembership {
79-
// TODO: not used yet.
80-
membership: Arc<EffectiveMembership<NID, N>>,
81-
},
82-
8370
/// Membership config changed, need to update replication streams.
8471
/// The Runtime has to close all old replications and start new ones.
8572
/// Because a replication stream should only report state for one membership config.
@@ -150,7 +137,6 @@ where
150137
(Command::LeaderCommit { already_committed, upto, }, Command::LeaderCommit { already_committed: b_committed, upto: b_upto, }, ) => already_committed == b_committed && upto == b_upto,
151138
(Command::FollowerCommit { already_committed, upto, }, Command::FollowerCommit { already_committed: b_committed, upto: b_upto, }, ) => already_committed == b_committed && upto == b_upto,
152139
(Command::Replicate { target, req }, Command::Replicate { target: b_target, req: other_req, }, ) => target == b_target && req == other_req,
153-
(Command::UpdateMembership { membership }, Command::UpdateMembership { membership: b }, ) => membership == b,
154140
(Command::RebuildReplicationStreams { targets }, Command::RebuildReplicationStreams { targets: b }, ) => targets == b,
155141
(Command::UpdateProgressMetrics { target, matching }, Command::UpdateProgressMetrics { target: b_target, matching: b_matching, }, ) => target == b_target && matching == b_matching,
156142
(Command::SaveVote { vote }, Command::SaveVote { vote: b }) => vote == b,
@@ -187,7 +173,6 @@ where
187173
Command::LeaderCommit { .. } => flags.set_data_changed(),
188174
Command::FollowerCommit { .. } => flags.set_data_changed(),
189175
Command::Replicate { .. } => {}
190-
Command::UpdateMembership { .. } => flags.set_cluster_changed(),
191176
Command::RebuildReplicationStreams { .. } => flags.set_replication_changed(),
192177
Command::UpdateProgressMetrics { .. } => flags.set_replication_changed(),
193178
Command::SaveVote { .. } => flags.set_data_changed(),

‎openraft/src/engine/engine_impl.rs

+1-2
Original file line numberDiff line numberDiff line change
@@ -158,10 +158,9 @@ where
158158
tracing::debug!("update effective membership: log_id:{} {}", log_id, m.summary());
159159

160160
let em = EffectiveMembership::new_arc(Some(log_id), m.clone());
161-
self.state.membership_state.append(em.clone());
161+
self.state.membership_state.append(em);
162162

163163
self.output.push_command(Command::AppendEntry { entry });
164-
self.output.push_command(Command::UpdateMembership { membership: em });
165164

166165
self.server_state_handler().update_server_state_if_changed();
167166

‎openraft/src/engine/handler/following_handler/do_append_entries_test.rs

+18-28
Original file line numberDiff line numberDiff line change
@@ -164,21 +164,16 @@ fn test_follower_do_append_entries_one_membership_entry() -> anyhow::Result<()>
164164
"not in membership, become learner"
165165
);
166166
assert_eq!(
167-
vec![
168-
Command::UpdateMembership {
169-
membership: Arc::new(EffectiveMembership::new(Some(log_id(3, 5)), m34())),
170-
},
171-
Command::AppendInputEntries {
172-
entries: vec![
173-
//
174-
blank_ent(3, 4),
175-
Entry::<UTCfg> {
176-
log_id: log_id(3, 5),
177-
payload: EntryPayload::<UTCfg>::Membership(m34()),
178-
},
179-
]
180-
},
181-
],
167+
vec![Command::AppendInputEntries {
168+
entries: vec![
169+
//
170+
blank_ent(3, 4),
171+
Entry::<UTCfg> {
172+
log_id: log_id(3, 5),
173+
payload: EntryPayload::<UTCfg>::Membership(m34()),
174+
},
175+
]
176+
},],
182177
eng.output.take_commands()
183178
);
184179

@@ -230,19 +225,14 @@ fn test_follower_do_append_entries_three_membership_entries() -> anyhow::Result<
230225
"in membership, become follower"
231226
);
232227
assert_eq!(
233-
vec![
234-
Command::UpdateMembership {
235-
membership: Arc::new(EffectiveMembership::new(Some(log_id(4, 7)), m45())),
236-
},
237-
Command::AppendInputEntries {
238-
entries: vec![
239-
blank_ent(3, 4),
240-
Entry::<UTCfg>::new_membership(log_id(3, 5), m01()),
241-
Entry::<UTCfg>::new_membership(log_id(4, 6), m34()),
242-
Entry::<UTCfg>::new_membership(log_id(4, 7), m45()),
243-
]
244-
},
245-
],
228+
vec![Command::AppendInputEntries {
229+
entries: vec![
230+
blank_ent(3, 4),
231+
Entry::<UTCfg>::new_membership(log_id(3, 5), m01()),
232+
Entry::<UTCfg>::new_membership(log_id(4, 6), m34()),
233+
Entry::<UTCfg>::new_membership(log_id(4, 7), m45()),
234+
]
235+
},],
246236
eng.output.take_commands()
247237
);
248238

‎openraft/src/engine/handler/following_handler/install_snapshot_test.rs

-9
Original file line numberDiff line numberDiff line change
@@ -144,9 +144,6 @@ fn test_install_snapshot_not_conflict() -> anyhow::Result<()> {
144144
);
145145
assert_eq!(
146146
vec![
147-
Command::UpdateMembership {
148-
membership: Arc::new(EffectiveMembership::new(Some(log_id(1, 1)), m1234()))
149-
},
150147
//
151148
Command::InstallSnapshot {
152149
snapshot_meta: SnapshotMeta {
@@ -215,9 +212,6 @@ fn test_install_snapshot_conflict() -> anyhow::Result<()> {
215212
assert_eq!(
216213
vec![
217214
Command::DeleteConflictLog { since: log_id(2, 4) },
218-
Command::UpdateMembership {
219-
membership: Arc::new(EffectiveMembership::new(Some(log_id(1, 1)), m1234()))
220-
},
221215
//
222216
Command::InstallSnapshot {
223217
snapshot_meta: SnapshotMeta {
@@ -265,9 +259,6 @@ fn test_install_snapshot_advance_last_log_id() -> anyhow::Result<()> {
265259
);
266260
assert_eq!(
267261
vec![
268-
Command::UpdateMembership {
269-
membership: Arc::new(EffectiveMembership::new(Some(log_id(1, 1)), m1234()))
270-
},
271262
//
272263
Command::InstallSnapshot {
273264
snapshot_meta: SnapshotMeta {

‎openraft/src/engine/handler/following_handler/mod.rs

+2-10
Original file line numberDiff line numberDiff line change
@@ -188,8 +188,7 @@ where
188188
self.output.push_command(Command::DeleteConflictLog { since: since_log_id });
189189

190190
let changed = self.state.membership_state.truncate(since);
191-
if let Some(c) = changed {
192-
self.output.push_command(Command::UpdateMembership { membership: c });
191+
if let Some(_c) = changed {
193192
self.server_state_handler().update_server_state_if_changed();
194193
}
195194
}
@@ -219,10 +218,6 @@ where
219218
"updated membership state"
220219
);
221220

222-
self.output.push_command(Command::UpdateMembership {
223-
membership: self.state.membership_state.effective().clone(),
224-
});
225-
226221
self.server_state_handler().update_server_state_if_changed();
227222
}
228223

@@ -236,10 +231,7 @@ where
236231
// TODO: if effective membership changes, call `update_replication()`, if a follower has replication
237232
// streams. Now we don't have replication streams for follower, so it's ok to not call
238233
// `update_replication()`.
239-
let effective_changed = self.state.membership_state.update_committed(m);
240-
if let Some(c) = effective_changed {
241-
self.output.push_command(Command::UpdateMembership { membership: c })
242-
}
234+
let _effective_changed = self.state.membership_state.update_committed(m);
243235

244236
self.server_state_handler().update_server_state_if_changed();
245237
}

‎openraft/src/engine/handler/following_handler/truncate_logs_test.rs

-6
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,6 @@ fn test_truncate_logs_since_3() -> anyhow::Result<()> {
6767
vec![
6868
//
6969
Command::DeleteConflictLog { since: log_id(2, 3) },
70-
Command::UpdateMembership {
71-
membership: Arc::new(EffectiveMembership::new(Some(log_id(1, 1)), m01()))
72-
},
7370
],
7471
eng.output.take_commands()
7572
);
@@ -186,9 +183,6 @@ fn test_truncate_logs_revert_effective_membership() -> anyhow::Result<()> {
186183
vec![
187184
//
188185
Command::DeleteConflictLog { since: log_id(4, 4) },
189-
Command::UpdateMembership {
190-
membership: Arc::new(EffectiveMembership::new(Some(log_id(2, 3)), m01()))
191-
},
192186
],
193187
eng.output.take_commands()
194188
);

‎openraft/src/engine/handler/following_handler/update_committed_membership_test.rs

+1-7
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ use maplit::btreeset;
55
use crate::core::ServerState;
66
use crate::engine::testing::UTCfg;
77
use crate::engine::CEngine;
8-
use crate::engine::Command;
98
use crate::engine::Engine;
109
use crate::testing::log_id;
1110
use crate::EffectiveMembership;
@@ -52,12 +51,7 @@ fn test_update_committed_membership_at_index_4() -> anyhow::Result<()> {
5251
eng.state.membership_state
5352
);
5453
assert_eq!(ServerState::Learner, eng.state.server_state);
55-
assert_eq!(
56-
vec![Command::UpdateMembership {
57-
membership: Arc::new(EffectiveMembership::new(Some(log_id(3, 4)), m34())),
58-
},],
59-
eng.output.take_commands()
60-
);
54+
assert_eq!(true, eng.output.take_commands().is_empty());
6155

6256
Ok(())
6357
}

‎openraft/src/engine/handler/leader_handler/append_entries_test.rs

-18
Original file line numberDiff line numberDiff line change
@@ -278,12 +278,6 @@ fn test_leader_append_entries_fast_commit_upto_membership_entry() -> anyhow::Res
278278
already_committed: Some(log_id(0, 0)),
279279
upto: LogId::new(CommittedLeaderId::new(3, 1), 4)
280280
},
281-
Command::UpdateMembership {
282-
membership: Arc::new(EffectiveMembership::new(
283-
Some(LogId::new(CommittedLeaderId::new(3, 1), 5)),
284-
m34()
285-
)),
286-
},
287281
Command::RebuildReplicationStreams {
288282
targets: vec![(3, ProgressEntry::empty(7)), (4, ProgressEntry::empty(7))]
289283
},
@@ -364,12 +358,6 @@ fn test_leader_append_entries_fast_commit_membership_no_voter_change() -> anyhow
364358
already_committed: Some(log_id(0, 0)),
365359
upto: LogId::new(CommittedLeaderId::new(3, 1), 4)
366360
},
367-
Command::UpdateMembership {
368-
membership: Arc::new(EffectiveMembership::new(
369-
Some(LogId::new(CommittedLeaderId::new(3, 1), 5)),
370-
m1_2()
371-
)),
372-
},
373361
Command::RebuildReplicationStreams {
374362
targets: vec![(2, ProgressEntry::empty(7))]
375363
},
@@ -448,12 +436,6 @@ fn test_leader_append_entries_fast_commit_if_membership_voter_change_to_1() -> a
448436
blank_ent(3, 6),
449437
]
450438
},
451-
Command::UpdateMembership {
452-
membership: Arc::new(EffectiveMembership::new(
453-
Some(LogId::new(CommittedLeaderId::new(3, 1), 5)),
454-
m1_2()
455-
)),
456-
},
457439
Command::RebuildReplicationStreams {
458440
targets: vec![(2, ProgressEntry::empty(7))]
459441
},

‎openraft/src/engine/handler/replication_handler/append_membership_test.rs

-3
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,6 @@ fn test_leader_append_membership_for_leader() -> anyhow::Result<()> {
7979
assert_eq!(
8080
vec![
8181
//
82-
Command::UpdateMembership {
83-
membership: Arc::new(EffectiveMembership::new(Some(log_id(3, 4)), m34())),
84-
},
8582
Command::RebuildReplicationStreams {
8683
targets: vec![(3, ProgressEntry::empty(0)), (4, ProgressEntry::empty(0))], /* node-2 is leader,
8784
* won't be removed */

‎openraft/src/engine/handler/replication_handler/mod.rs

-3
Original file line numberDiff line numberDiff line change
@@ -94,9 +94,6 @@ where
9494

9595
self.state.membership_state.append(EffectiveMembership::new_arc(Some(*log_id), m.clone()));
9696

97-
let em = self.state.membership_state.effective();
98-
self.output.push_command(Command::UpdateMembership { membership: em.clone() });
99-
10097
// TODO(9): currently only a leader has replication setup.
10198
// It's better to setup replication for both leader and candidate.
10299
// e.g.: if self.internal_server_state.is_leading() {

‎openraft/src/engine/tests/append_entries_test.rs

-12
Original file line numberDiff line numberDiff line change
@@ -159,9 +159,6 @@ fn test_append_entries_prev_log_id_conflict() -> anyhow::Result<()> {
159159
vote: Vote::new_committed(2, 1)
160160
},
161161
Command::DeleteConflictLog { since: log_id(1, 2) },
162-
Command::UpdateMembership {
163-
membership: Arc::new(EffectiveMembership::new(Some(log_id(1, 1)), m01()))
164-
},
165162
],
166163
eng.output.take_commands()
167164
);
@@ -202,9 +199,6 @@ fn test_append_entries_prev_log_id_is_committed() -> anyhow::Result<()> {
202199
vote: Vote::new_committed(2, 1)
203200
},
204201
Command::DeleteConflictLog { since: log_id(1, 2) },
205-
Command::UpdateMembership {
206-
membership: Arc::new(EffectiveMembership::new(Some(log_id(1, 1)), m01()))
207-
},
208202
Command::AppendInputEntries {
209203
entries: vec![blank_ent(2, 2)]
210204
},
@@ -298,12 +292,6 @@ fn test_append_entries_conflict() -> anyhow::Result<()> {
298292
vote: Vote::new_committed(2, 1)
299293
},
300294
Command::DeleteConflictLog { since: log_id(2, 3) },
301-
Command::UpdateMembership {
302-
membership: Arc::new(EffectiveMembership::new(Some(log_id(1, 1)), m01()))
303-
},
304-
Command::UpdateMembership {
305-
membership: Arc::new(EffectiveMembership::new(Some(log_id(3, 3)), m34()))
306-
},
307295
Command::AppendInputEntries {
308296
entries: vec![Entry::new_membership(log_id(3, 3), m34())]
309297
},

‎openraft/src/engine/tests/command_test.rs

-2
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ use crate::progress::Inflight;
66
use crate::raft::VoteRequest;
77
use crate::raft_types::MetricsChangeFlags;
88
use crate::testing::log_id;
9-
use crate::EffectiveMembership;
109
use crate::Entry;
1110
use crate::Membership;
1211
use crate::SnapshotMeta;
@@ -35,7 +34,6 @@ fn test_command_update_metrics_flags() -> anyhow::Result<()> {
3534
t(Command::LeaderCommit { already_committed: None, upto: log_id(1,2) }, false, true, false);
3635
t(Command::FollowerCommit { already_committed: None, upto: log_id(1,2) }, false, true, false);
3736
t(Command::Replicate { target: 3, req: Inflight::None }, false, false, false);
38-
t(Command::UpdateMembership{ membership: EffectiveMembership::new_arc(Some(log_id(1,1)), Membership::new(vec![], ()) ) }, false, false, true);
3937
t(Command::RebuildReplicationStreams{ targets: vec![] }, true, false, false);
4038
t(Command::UpdateProgressMetrics{ target: 0, matching: log_id(1,2), }, true, false, false);
4139
t(Command::SaveVote{ vote: Vote::new(1,2) }, false, true, false);

‎openraft/src/engine/tests/initialize_test.rs

-14
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,6 @@ fn test_initialize_single_node() -> anyhow::Result<()> {
7474
Command::AppendEntry {
7575
entry: Entry::<Config>::new_membership(LogId::default(), m1())
7676
},
77-
Command::UpdateMembership {
78-
membership: eng.state.membership_state.effective().clone()
79-
},
8077
// When update the effective membership, the engine set it to Follower.
8178
// But when initializing, it will switch to Candidate at once, in the last output
8279
// command.
@@ -145,24 +142,13 @@ fn test_initialize() -> anyhow::Result<()> {
145142
assert_eq!(Some(&log_id0), eng.state.last_log_id());
146143

147144
assert_eq!(ServerState::Candidate, eng.state.server_state);
148-
assert_eq!(
149-
MetricsChangeFlags {
150-
replication: false,
151-
local_data: true,
152-
cluster: true,
153-
},
154-
eng.output.metrics_flags
155-
);
156145
assert_eq!(&m12(), eng.state.membership_state.effective().membership());
157146

158147
assert_eq!(
159148
vec![
160149
Command::AppendEntry {
161150
entry: Entry::new_membership(LogId::default(), m12())
162151
},
163-
Command::UpdateMembership {
164-
membership: eng.state.membership_state.effective().clone()
165-
},
166152
// When update the effective membership, the engine set it to Follower.
167153
// But when initializing, it will switch to Candidate at once, in the last output
168154
// command.

0 commit comments

Comments
 (0)