Skip to content

Commit

Permalink
Change: change LeaderId<NID:NodeId> to LeaderId<C:RaftTypeConfig>
Browse files Browse the repository at this point in the history
This refactoring moves LeaderId from a per-NodeId type to a per-TypeConfig type,
to make it consistent with `RaftTypeConfig` usage across the codebase.

- Part of: databendlabs#1278

Upgrade tip:

LeaderId is now parameterized by `RaftTypeConfig` instead of `NodeId`

- Change `LeaderId<NodeId>` to `LeaderId<C> where C: RaftTypeConfig`, for
  example, change `LeaderId<u64>` to `LeaderId<YourTypeConfig>`.

- Change `CommittedLeaderId<NodeId>` to `CommittedLeaderId<RaftTypeConfig>`.
  • Loading branch information
drmingdrmer committed Dec 25, 2024
1 parent 4d36290 commit 62c9b2e
Show file tree
Hide file tree
Showing 10 changed files with 77 additions and 53 deletions.
10 changes: 5 additions & 5 deletions examples/raft-kv-memstore-grpc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,15 @@ pub mod typ {
pub type ClientWriteResponse = openraft::raft::ClientWriteResponse<TypeConfig>;
}

impl From<protobuf::LeaderId> for LeaderId<NodeId> {
impl From<protobuf::LeaderId> for LeaderId<TypeConfig> {
fn from(proto_leader_id: protobuf::LeaderId) -> Self {
LeaderId::new(proto_leader_id.term, proto_leader_id.node_id)
}
}

impl From<protobuf::Vote> for typ::Vote {
fn from(proto_vote: protobuf::Vote) -> Self {
let leader_id: LeaderId<NodeId> = proto_vote.leader_id.unwrap().into();
let leader_id: LeaderId<TypeConfig> = proto_vote.leader_id.unwrap().into();
if proto_vote.committed {
typ::Vote::new_committed(leader_id.term, leader_id.node_id)
} else {
Expand All @@ -75,7 +75,7 @@ impl From<protobuf::Vote> for typ::Vote {

impl From<protobuf::LogId> for LogId<TypeConfig> {
fn from(proto_log_id: protobuf::LogId) -> Self {
let leader_id: LeaderId<NodeId> = proto_log_id.leader_id.unwrap().into();
let leader_id: LeaderId<TypeConfig> = proto_log_id.leader_id.unwrap().into();
LogId::new(leader_id, proto_log_id.index)
}
}
Expand All @@ -96,8 +96,8 @@ impl From<protobuf::VoteResponse> for VoteResponse<TypeConfig> {
}
}

impl From<LeaderId<NodeId>> for protobuf::LeaderId {
fn from(leader_id: LeaderId<NodeId>) -> Self {
impl From<LeaderId<TypeConfig>> for protobuf::LeaderId {
fn from(leader_id: LeaderId<TypeConfig>) -> Self {
protobuf::LeaderId {
term: leader_id.term,
node_id: leader_id.node_id,
Expand Down
6 changes: 3 additions & 3 deletions openraft/src/log_id/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub struct LogId<C>
where C: RaftTypeConfig
{
/// The id of the leader that proposed this log
pub leader_id: CommittedLeaderId<C::NodeId>,
pub leader_id: CommittedLeaderId<C>,
/// The index of a log in the storage.
///
/// Log index is a consecutive integer.
Expand Down Expand Up @@ -63,12 +63,12 @@ impl<C> LogId<C>
where C: RaftTypeConfig
{
/// Creates a log id proposed by a committed leader with `leader_id` at the given index.
pub fn new(leader_id: CommittedLeaderId<C::NodeId>, index: u64) -> Self {
pub fn new(leader_id: CommittedLeaderId<C>, index: u64) -> Self {
LogId { leader_id, index }
}

/// Returns the leader id that proposed this log.
pub fn committed_leader_id(&self) -> &CommittedLeaderId<C::NodeId> {
pub fn committed_leader_id(&self) -> &CommittedLeaderId<C> {
&self.leader_id
}
}
2 changes: 1 addition & 1 deletion openraft/src/log_id/raft_log_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ where C: RaftTypeConfig
/// For example, a leader id in standard raft is `(term, node_id)`, but a log id does not have
/// to store the `node_id`, because in standard raft there is at most one leader that can be
/// established.
fn leader_id(&self) -> &CommittedLeaderId<C::NodeId> {
fn leader_id(&self) -> &CommittedLeaderId<C> {
self.get_log_id().committed_leader_id()
}

Expand Down
4 changes: 2 additions & 2 deletions openraft/src/type_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ pub mod alias {
// Usually used types
pub type LogIdOf<C> = crate::LogId<C>;
pub type VoteOf<C> = crate::Vote<C>;
pub type LeaderIdOf<C> = crate::LeaderId<NodeIdOf<C>>;
pub type CommittedLeaderIdOf<C> = crate::CommittedLeaderId<NodeIdOf<C>>;
pub type LeaderIdOf<C> = crate::LeaderId<C>;
pub type CommittedLeaderIdOf<C> = crate::CommittedLeaderId<C>;
pub type SerdeInstantOf<C> = crate::metrics::SerdeInstant<InstantOf<C>>;
}
2 changes: 1 addition & 1 deletion openraft/src/vote/committed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ where C: RaftTypeConfig
Self { vote }
}

pub(crate) fn committed_leader_id(&self) -> CommittedLeaderId<C::NodeId> {
pub(crate) fn committed_leader_id(&self) -> CommittedLeaderId<C> {
self.vote.leader_id().to_committed()
}

Expand Down
2 changes: 1 addition & 1 deletion openraft/src/vote/leader_id/impl_into_leader_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::vote::leader_id::CommittedLeaderId;
use crate::vote::Vote;
use crate::RaftTypeConfig;

impl<C> From<Vote<C>> for CommittedLeaderId<C::NodeId>
impl<C> From<Vote<C>> for CommittedLeaderId<C>
where C: RaftTypeConfig
{
fn from(vote: Vote<C>) -> Self {
Expand Down
37 changes: 21 additions & 16 deletions openraft/src/vote/leader_id/leader_id_adv.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::fmt;

use crate::NodeId;
use crate::RaftTypeConfig;

/// LeaderId is identifier of a `leader`.
///
Expand All @@ -14,73 +14,78 @@ use crate::NodeId;
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
#[derive(PartialOrd, Ord)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize), serde(bound = ""))]
pub struct LeaderId<NID>
where NID: NodeId
pub struct LeaderId<C>
where C: RaftTypeConfig
{
pub term: u64,
pub node_id: NID,
pub node_id: C::NodeId,
}

impl<NID: NodeId> LeaderId<NID> {
pub fn new(term: u64, node_id: NID) -> Self {
impl<C> LeaderId<C>
where C: RaftTypeConfig
{
pub fn new(term: u64, node_id: C::NodeId) -> Self {
Self { term, node_id }
}

pub fn get_term(&self) -> u64 {
self.term
}

pub fn voted_for(&self) -> Option<NID> {
pub fn voted_for(&self) -> Option<C::NodeId> {
Some(self.node_id.clone())
}

#[allow(clippy::wrong_self_convention)]
pub(crate) fn to_committed(&self) -> CommittedLeaderId<NID> {
pub(crate) fn to_committed(&self) -> CommittedLeaderId<C> {
self.clone()
}

/// Return if it is the same leader as the committed leader id.
///
/// A committed leader may have less info than a non-committed.
pub(crate) fn is_same_as_committed(&self, other: &CommittedLeaderId<NID>) -> bool {
pub(crate) fn is_same_as_committed(&self, other: &CommittedLeaderId<C>) -> bool {
self == other
}
}

impl<NID: NodeId> fmt::Display for LeaderId<NID> {
impl<C> fmt::Display for LeaderId<C>
where C: RaftTypeConfig
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "T{}-N{}", self.term, self.node_id)
}
}

/// The unique identifier of a leader that is already granted by a quorum in phase-1(voting).
///
/// [`CommittedLeaderId`] may contains less information than [`LeaderId`], because it implies the
/// [`CommittedLeaderId`] may contain less information than [`LeaderId`], because it implies the
/// constraint that **a quorum has granted it**.
///
/// For a total order `LeaderId`, the [`CommittedLeaderId`] is the same.
///
/// For a partial order `LeaderId`, we know that all of the granted
/// For a partial order `LeaderId`, we know that all the granted
/// leader-id must be a total order set. Therefor once it is granted by a quorum, it only keeps the
/// information that makes leader-ids a correct total order set, e.g., in standard raft, `voted_for:
/// Option<node_id>` can be removed from `(term, voted_for)` once it is granted. This is why
/// standard raft stores just a `term` in log entry.
pub type CommittedLeaderId<NID> = LeaderId<NID>;
pub type CommittedLeaderId<C> = LeaderId<C>;

#[cfg(test)]
mod tests {
use crate::engine::testing::UTConfig;
use crate::LeaderId;

#[cfg(feature = "serde")]
#[test]
fn test_committed_leader_id_serde() -> anyhow::Result<()> {
use crate::CommittedLeaderId;

let c = CommittedLeaderId::<u32>::new(5, 10);
let c = CommittedLeaderId::<UTConfig>::new(5, 10);
let s = serde_json::to_string(&c)?;
assert_eq!(r#"{"term":5,"node_id":10}"#, s);

let c2: CommittedLeaderId<u32> = serde_json::from_str(&s)?;
let c2: CommittedLeaderId<UTConfig> = serde_json::from_str(&s)?;
assert_eq!(CommittedLeaderId::new(5, 10), c2);

Ok(())
Expand All @@ -89,7 +94,7 @@ mod tests {
#[test]
fn test_leader_id_partial_order() -> anyhow::Result<()> {
#[allow(clippy::redundant_closure)]
let lid = |term, node_id| LeaderId::<u64>::new(term, node_id);
let lid = |term, node_id| LeaderId::<UTConfig>::new(term, node_id);

// Compare term first
assert!(lid(2, 2) > lid(1, 2));
Expand Down
48 changes: 30 additions & 18 deletions openraft/src/vote/leader_id/leader_id_std.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,21 @@ use std::marker::PhantomData;

use crate::display_ext::DisplayOptionExt;
use crate::NodeId;

Check warning on line 6 in openraft/src/vote/leader_id/leader_id_std.rs

View workflow job for this annotation

GitHub Actions / openraft-feature-test (nightly, single-term-leader,serde,singlethreaded)

unused import: `crate::NodeId`

Check warning on line 6 in openraft/src/vote/leader_id/leader_id_std.rs

View workflow job for this annotation

GitHub Actions / openraft-feature-test (nightly, single-term-leader)

unused import: `crate::NodeId`

Check warning on line 6 in openraft/src/vote/leader_id/leader_id_std.rs

View workflow job for this annotation

GitHub Actions / tests-feature-test (nightly, single-term-leader)

unused import: `crate::NodeId`

Check warning on line 6 in openraft/src/vote/leader_id/leader_id_std.rs

View workflow job for this annotation

GitHub Actions / openraft-test (nightly, 0, single-term-leader)

unused import: `crate::NodeId`

Check warning on line 6 in openraft/src/vote/leader_id/leader_id_std.rs

View workflow job for this annotation

GitHub Actions / openraft-test (nightly, 0, single-term-leader)

unused import: `crate::NodeId`

Check failure on line 6 in openraft/src/vote/leader_id/leader_id_std.rs

View workflow job for this annotation

GitHub Actions / lint

unused import: `crate::NodeId`
use crate::RaftTypeConfig;

#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize), serde(bound = ""))]
pub struct LeaderId<NID>
where NID: NodeId
pub struct LeaderId<C>
where C: RaftTypeConfig
{
pub term: u64,

pub voted_for: Option<NID>,
pub voted_for: Option<C::NodeId>,
}

impl<NID: NodeId> PartialOrd for LeaderId<NID> {
impl<C> PartialOrd for LeaderId<C>
where C: RaftTypeConfig
{
#[inline]
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
match PartialOrd::partial_cmp(&self.term, &other.term) {
Expand All @@ -39,8 +42,10 @@ impl<NID: NodeId> PartialOrd for LeaderId<NID> {
}
}

impl<NID: NodeId> LeaderId<NID> {
pub fn new(term: u64, node_id: NID) -> Self {
impl<C> LeaderId<C>
where C: RaftTypeConfig
{
pub fn new(term: u64, node_id: C::NodeId) -> Self {
Self {
term,
voted_for: Some(node_id),
Expand All @@ -51,24 +56,26 @@ impl<NID: NodeId> LeaderId<NID> {
self.term
}

pub fn voted_for(&self) -> Option<NID> {
pub fn voted_for(&self) -> Option<C::NodeId> {
self.voted_for.clone()
}

#[allow(clippy::wrong_self_convention)]
pub(crate) fn to_committed(&self) -> CommittedLeaderId<NID> {
CommittedLeaderId::new(self.term, NID::default())
pub(crate) fn to_committed(&self) -> CommittedLeaderId<C> {
CommittedLeaderId::new(self.term, C::NodeId::default())
}

/// Return if it is the same leader as the committed leader id.
///
/// A committed leader may have less info than a non-committed.
pub(crate) fn is_same_as_committed(&self, other: &CommittedLeaderId<NID>) -> bool {
pub(crate) fn is_same_as_committed(&self, other: &CommittedLeaderId<C>) -> bool {
self.term == other.term
}
}

impl<NID: NodeId> fmt::Display for LeaderId<NID> {
impl<C> fmt::Display for LeaderId<C>
where C: RaftTypeConfig
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "T{}-N{}", self.term, self.voted_for.display())
}
Expand All @@ -78,19 +85,23 @@ impl<NID: NodeId> fmt::Display for LeaderId<NID> {
#[derive(PartialOrd, Ord)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize), serde(bound = ""))]
#[cfg_attr(feature = "serde", serde(transparent))]
pub struct CommittedLeaderId<NID> {
pub struct CommittedLeaderId<C> {
pub term: u64,
p: PhantomData<NID>,
p: PhantomData<C>,
}

impl<NID: NodeId> fmt::Display for CommittedLeaderId<NID> {
impl<C> fmt::Display for CommittedLeaderId<C>
where C: RaftTypeConfig
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.term)
}
}

impl<NID: NodeId> CommittedLeaderId<NID> {
pub fn new(term: u64, node_id: NID) -> Self {
impl<C> CommittedLeaderId<C>
where C: RaftTypeConfig
{
pub fn new(term: u64, node_id: C::NodeId) -> Self {
let _ = node_id;
Self { term, p: PhantomData }
}
Expand All @@ -99,6 +110,7 @@ impl<NID: NodeId> CommittedLeaderId<NID> {
#[cfg(test)]
#[allow(clippy::nonminimal_bool)]
mod tests {
use crate::engine::testing::UTConfig;
use crate::LeaderId;

#[cfg(feature = "serde")]
Expand All @@ -120,9 +132,9 @@ mod tests {
#[allow(clippy::neg_cmp_op_on_partial_ord)]
fn test_leader_id_partial_order() -> anyhow::Result<()> {
#[allow(clippy::redundant_closure)]
let lid = |term, node_id| LeaderId::<u64>::new(term, node_id);
let lid = |term, node_id| LeaderId::<UTConfig>::new(term, node_id);

let lid_none = |term| LeaderId::<u64> { term, voted_for: None };
let lid_none = |term| LeaderId::<UTConfig> { term, voted_for: None };

// Compare term first
assert!(lid(2, 2) > lid(1, 2));
Expand Down
4 changes: 2 additions & 2 deletions openraft/src/vote/ref_vote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ use crate::RaftTypeConfig;
pub(crate) struct RefVote<'a, C>
where C: RaftTypeConfig
{
pub(crate) leader_id: &'a LeaderId<C::NodeId>,
pub(crate) leader_id: &'a LeaderId<C>,
pub(crate) committed: bool,
}

impl<'a, C> RefVote<'a, C>
where C: RaftTypeConfig
{
pub(crate) fn new(leader_id: &'a LeaderId<C::NodeId>, committed: bool) -> Self {
pub(crate) fn new(leader_id: &'a LeaderId<C>, committed: bool) -> Self {
Self { leader_id, committed }
}

Expand Down
15 changes: 11 additions & 4 deletions openraft/src/vote/vote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,22 @@ use crate::LeaderId;
use crate::RaftTypeConfig;

/// `Vote` represent the privilege of a node.
#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
#[derive(Debug, Clone, Default, PartialEq, Eq)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize), serde(bound = ""))]
pub struct Vote<C: RaftTypeConfig> {
/// The id of the node that tries to become the leader.
pub leader_id: LeaderId<C::NodeId>,
pub leader_id: LeaderId<C>,

pub committed: bool,
}

impl<C> Copy for Vote<C>
where
C: RaftTypeConfig,
C::NodeId: Copy,
{
}

impl<C> PartialOrd for Vote<C>
where C: RaftTypeConfig
{
Expand Down Expand Up @@ -91,11 +98,11 @@ where C: RaftTypeConfig
/// Return the [`LeaderId`] this vote represents for.
///
/// The leader may or may not be granted by a quorum.
pub fn leader_id(&self) -> &LeaderId<C::NodeId> {
pub fn leader_id(&self) -> &LeaderId<C> {
&self.leader_id
}

pub(crate) fn is_same_leader(&self, leader_id: &CommittedLeaderId<C::NodeId>) -> bool {
pub(crate) fn is_same_leader(&self, leader_id: &CommittedLeaderId<C>) -> bool {
self.leader_id().is_same_as_committed(leader_id)
}
}
Expand Down

0 comments on commit 62c9b2e

Please sign in to comment.