Skip to content

Commit

Permalink
Refactor: Deprecate LogId::leader_id()
Browse files Browse the repository at this point in the history
The method `LogId::leader_id()` returns a `&CommittedLeaderId` instead
of a `LeaderId`, which can be misleading.

This commit deprecates `leader_id()` and recommends using the more
descriptive `LogId::committed_leader_id()` method instead.
  • Loading branch information
drmingdrmer committed Jan 27, 2025
1 parent 427ac06 commit 049036d
Show file tree
Hide file tree
Showing 15 changed files with 36 additions and 29 deletions.
2 changes: 1 addition & 1 deletion cluster_benchmark/tests/benchmark/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ impl RaftSnapshotBuilder<TypeConfig> for Arc<StateMachineStore> {
let snapshot_idx = self.snapshot_idx.fetch_add(1, Ordering::Relaxed);

let snapshot_id = if let Some(last) = last_applied_log {
format!("{}-{}-{}", last.leader_id(), last.index(), snapshot_idx)
format!("{}-{}-{}", last.committed_leader_id(), last.index(), snapshot_idx)
} else {
format!("--{}", snapshot_idx)
};
Expand Down
2 changes: 1 addition & 1 deletion examples/raft-kv-memstore-grpc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ impl From<pb::VoteResponse> for VoteResponse {
impl From<LogId> for pb::LogId {
fn from(log_id: LogId) -> Self {
pb::LogId {
term: *log_id.leader_id(),
term: *log_id.committed_leader_id(),
index: log_id.index(),
}
}
Expand Down
2 changes: 1 addition & 1 deletion examples/raft-kv-memstore-grpc/src/store/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ impl RaftSnapshotBuilder<TypeConfig> for Arc<StateMachineStore> {
};

let snapshot_id = if let Some(last) = last_applied_log {
format!("{}-{}-{}", last.leader_id(), last.index(), snapshot_idx)
format!("{}-{}-{}", last.committed_leader_id(), last.index(), snapshot_idx)
} else {
format!("--{}", snapshot_idx)
};
Expand Down
2 changes: 1 addition & 1 deletion examples/raft-kv-memstore-network-v2/src/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ impl RaftSnapshotBuilder<TypeConfig> for Arc<StateMachineStore> {
};

let snapshot_id = if let Some(last) = last_applied_log {
format!("{}-{}-{}", last.leader_id(), last.index(), snapshot_idx)
format!("{}-{}-{}", last.committed_leader_id(), last.index(), snapshot_idx)
} else {
format!("--{}", snapshot_idx)
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ impl RaftSnapshotBuilder<TypeConfig> for Arc<StateMachineStore> {
};

let snapshot_id = if let Some(last) = last_applied_log {
format!("{}-{}-{}", last.leader_id(), last.index(), snapshot_idx)
format!("{}-{}-{}", last.committed_leader_id(), last.index(), snapshot_idx)
} else {
format!("--{}", snapshot_idx)
};
Expand Down
2 changes: 1 addition & 1 deletion examples/raft-kv-memstore-singlethreaded/src/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ impl RaftSnapshotBuilder<TypeConfig> for Rc<StateMachineStore> {
};

let snapshot_id = if let Some(last) = last_applied_log {
format!("{}-{}-{}", last.leader_id(), last.index(), snapshot_idx)
format!("{}-{}-{}", last.committed_leader_id(), last.index(), snapshot_idx)
} else {
format!("--{}", snapshot_idx)
};
Expand Down
2 changes: 1 addition & 1 deletion examples/raft-kv-memstore/src/store/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ impl RaftSnapshotBuilder<TypeConfig> for Arc<StateMachineStore> {

let snapshot_idx = self.snapshot_idx.fetch_add(1, Ordering::Relaxed) + 1;
let snapshot_id = if let Some(last) = last_applied_log {
format!("{}-{}-{}", last.leader_id(), last.index(), snapshot_idx)
format!("{}-{}-{}", last.committed_leader_id(), last.index(), snapshot_idx)
} else {
format!("--{}", snapshot_idx)
};
Expand Down
2 changes: 1 addition & 1 deletion examples/raft-kv-rocksdb/src/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ impl RaftSnapshotBuilder<TypeConfig> for StateMachineStore {
};

let snapshot_id = if let Some(last) = last_applied_log {
format!("{}-{}-{}", last.leader_id(), last.index(), self.snapshot_idx)
format!("{}-{}-{}", last.committed_leader_id(), last.index(), self.snapshot_idx)
} else {
format!("--{}", self.snapshot_idx)
};
Expand Down
2 changes: 1 addition & 1 deletion examples/rocksstore/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ impl RaftSnapshotBuilder<TypeConfig> for RocksStateMachine {
let snapshot_idx: u64 = rand::thread_rng().gen_range(0..1000);

let snapshot_id = if let Some(last) = last_applied_log {
format!("{}-{}-{}", last.leader_id(), last.index(), snapshot_idx)
format!("{}-{}-{}", last.committed_leader_id(), last.index(), snapshot_idx)
} else {
format!("--{}", snapshot_idx)
};
Expand Down
36 changes: 21 additions & 15 deletions openraft/src/engine/log_id_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,16 +71,16 @@ where C: RaftTypeConfig
};

// Case AA
if first.leader_id() == last.leader_id() {
if res.last().map(|x| x.leader_id()) < Some(first.leader_id()) {
if first.committed_leader_id() == last.committed_leader_id() {
if res.last().map(|x| x.committed_leader_id()) < Some(first.committed_leader_id()) {
res.push(first);
}
continue;
}

// Two adjacent logs with different leader_id, no need to binary search
if first.index() + 1 == last.index() {
if res.last().map(|x| x.leader_id()) < Some(first.leader_id()) {
if res.last().map(|x| x.committed_leader_id()) < Some(first.committed_leader_id()) {
res.push(first);
}
res.push(last);
Expand All @@ -89,13 +89,13 @@ where C: RaftTypeConfig

let mid = sto.get_log_id((first.index() + last.index()) / 2).await?;

if first.leader_id() == mid.leader_id() {
if first.committed_leader_id() == mid.committed_leader_id() {
// Case AAC
if res.last().map(|x| x.leader_id()) < Some(first.leader_id()) {
if res.last().map(|x| x.committed_leader_id()) < Some(first.committed_leader_id()) {
res.push(first);
}
stack.push((mid, last));
} else if mid.leader_id() == last.leader_id() {
} else if mid.committed_leader_id() == last.committed_leader_id() {
// Case ACC
stack.push((first, mid));
} else {
Expand Down Expand Up @@ -137,7 +137,7 @@ where C: RaftTypeConfig

if let Some(last) = new_ids.last() {
let last_id = last.get_log_id();
assert_eq!(last_id.leader_id(), first_id.leader_id());
assert_eq!(last_id.committed_leader_id(), first_id.committed_leader_id());

if last_id != first_id {
self.append(last_id.clone());
Expand All @@ -150,15 +150,15 @@ where C: RaftTypeConfig
// leader_id: Copy is feature gated
#[allow(clippy::clone_on_copy)]
pub(crate) fn extend<'a, LID: RaftLogId<C> + 'a>(&mut self, new_ids: &[LID]) {
let mut prev = self.last().map(|x| x.leader_id().clone());
let mut prev = self.last().map(|x| x.committed_leader_id().clone());

for x in new_ids.iter() {
let log_id = x.get_log_id();

if prev.as_ref() != Some(log_id.leader_id()) {
if prev.as_ref() != Some(log_id.committed_leader_id()) {
self.append(log_id.clone());

prev = Some(log_id.leader_id().clone());
prev = Some(log_id.committed_leader_id().clone());
}
}

Expand Down Expand Up @@ -204,7 +204,7 @@ where C: RaftTypeConfig

let last = &self.key_log_ids[l - 1];

if self.key_log_ids.get(l - 2).map(|x| x.leader_id()) == Some(last.leader_id()) {
if self.key_log_ids.get(l - 2).map(|x| x.committed_leader_id()) == Some(last.committed_leader_id()) {
// Replace the **last log id**.
self.key_log_ids[l - 1] = new_log_id;
return;
Expand Down Expand Up @@ -237,7 +237,7 @@ where C: RaftTypeConfig
// Add key log id if there is a gap between last.index and at - 1.
let last = self.key_log_ids.last();
if let Some(last) = last {
let (last_leader_id, last_index) = (last.leader_id().clone(), last.index());
let (last_leader_id, last_index) = (last.committed_leader_id().clone(), last.index());
if last_index < at - 1 {
self.append(LogIdOf::<C>::new(last_leader_id, at - 1));
}
Expand Down Expand Up @@ -284,12 +284,18 @@ where C: RaftTypeConfig
let res = self.key_log_ids.binary_search_by(|log_id| log_id.index().cmp(&index));

match res {
Ok(i) => Some(LogIdOf::<C>::new(self.key_log_ids[i].leader_id().clone(), index)),
Ok(i) => Some(LogIdOf::<C>::new(
self.key_log_ids[i].committed_leader_id().clone(),
index,
)),
Err(i) => {
if i == 0 || i == self.key_log_ids.len() {
None
} else {
Some(LogIdOf::<C>::new(self.key_log_ids[i - 1].leader_id().clone(), index))
Some(LogIdOf::<C>::new(
self.key_log_ids[i - 1].committed_leader_id().clone(),
index,
))
}
}
}
Expand Down Expand Up @@ -323,7 +329,7 @@ where C: RaftTypeConfig
}

// There are at most two(adjacent) key log ids with the same leader_id
if ks[l - 1].leader_id() == ks[l - 2].leader_id() {
if ks[l - 1].committed_leader_id() == ks[l - 2].committed_leader_id() {
LeaderLogIds::new_start_end(ks[l - 2].clone(), ks[l - 1].clone())
} else {
let last = self.last().cloned().unwrap();
Expand Down
3 changes: 2 additions & 1 deletion openraft/src/log_id/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl<C> Display for LogId<C>
where C: RaftTypeConfig
{
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
write!(f, "{}.{}", self.leader_id(), self.index())
write!(f, "{}.{}", self.committed_leader_id(), self.index())
}
}

Expand All @@ -72,6 +72,7 @@ where C: RaftTypeConfig
&self.leader_id
}

#[deprecated(since = "0.10.0", note = "Use `committed_leader_id` instead.")]
pub fn leader_id(&self) -> &CommittedLeaderIdOf<C> {
&self.leader_id
}
Expand Down
2 changes: 1 addition & 1 deletion openraft/src/storage/helper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ where

if !log_ids.is_empty() {
if let Some(purged) = purged {
if purged.leader_id() == log_ids[0].leader_id() {
if purged.committed_leader_id() == log_ids[0].committed_leader_id() {
if log_ids.len() >= 2 {
log_ids[0] = purged;
} else {
Expand Down
2 changes: 1 addition & 1 deletion stores/memstore/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ impl RaftSnapshotBuilder<TypeConfig> for Arc<MemStateMachine> {
};

let snapshot_id = if let Some(last) = last_applied_log {
format!("{}-{}-{}", last.leader_id(), last.index(), snapshot_idx)
format!("{}-{}-{}", last.committed_leader_id(), last.index(), snapshot_idx)
} else {
format!("--{}", snapshot_idx)
};
Expand Down
2 changes: 1 addition & 1 deletion tests/tests/append_entries/t11_append_inconsistent_log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ async fn append_inconsistent_log() -> Result<()> {
let logs = sto0.try_get_log_entries(60..=60).await?;
assert_eq!(
3,
logs.first().unwrap().log_id.leader_id().term,
logs.first().unwrap().log_id.committed_leader_id().term,
"log is overridden by leader logs"
);

Expand Down
2 changes: 1 addition & 1 deletion tests/tests/fixtures/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,7 @@ impl TypedRaftRouter {
}

assert_eq!(
&snap.meta.last_log_id.unwrap_or_default().leader_id().term,
&snap.meta.last_log_id.unwrap_or_default().committed_leader_id().term,
term,
"expected node {} to have snapshot with term {}, got {:?}",
id,
Expand Down

0 comments on commit 049036d

Please sign in to comment.