Skip to content

Commit

Permalink
Change: Replace MessageSummary with Display
Browse files Browse the repository at this point in the history
This commit eliminates redundant `MessageSummary` implementations, as
they were identical to the existing `Display` trait implementations. The
following types are affected by this change:

- Raft protocol types:

    - `VoteRequest`
    - `VoteResponse`
    - `AppendEntriesRequest`
    - `InstallSnapshotRequest`

- Application API types:

    - `ClientWriteResponse`
    - `SnapshotMeta`

- Base types:

    - `LogId`
    - `Vote`
    - `EntryPayload`
    - `Entry`

- Membership types:

    - `Membership`
    - `EffectiveMembership`
    - `StoredMembership`
    - `MembershipState`

- Metrics types:

    `RaftMetrics`
    `RaftDataMetrics`
    `RaftServerMetrics`

With this update, if possible, any usage of `MessageSummary` should be
replaced with `Display`.

`MessageSummary` provides addtional display abilities and can be used for
displaying `Option<T>` or `&[T]`:
- If `T` implements `Display`, then `T` implements `MessageSummary` too.
- `Option<T: MessageSummary>` implements `MessageSummary`.
- and `&[T]` where `T: MessageSummary` implements `MessageSummary`

#### Examples

```rust,ignore
use openraft::MessageSummary;
use openraft::testing::log_id;

let lid = log_id(1, 2, 3);

assert_eq!("1-2-3", lid.to_string(), "LogId is Display");
assert_eq!("1-2-3", lid.summary(), "Thus LogId is also MessageSummary");
assert_eq!("1-2-3", (&lid).summary(), "and its reference");
assert_eq!("Some(1-2-3)", Some(lid).summary(), "Option<LogId> can be displayed too");
assert_eq!("Some(1-2-3)", Some(&lid).summary(), "Option<&LogId> can be displayed too");

let slc = vec![lid, lid];
assert_eq!("1-2-3,1-2-3", slc.as_slice().summary(), "&[LogId] can be displayed too");

let slc = vec![&lid, &lid];
assert_eq!("1-2-3,1-2-3", slc.as_slice().summary(), "&[&LogId] can be displayed too");
```

Upgrade tip:

For instances of type `T`, replace any calls to `T::summary()` with the
standard `T::to_string()` method or the more general `format!("{}", t)`
pattern.

For convenience, when working with `Option<T>` or `&[T]`,
the `summary()` method can still be utilized to generate string
representations of these types.
  • Loading branch information
drmingdrmer committed Mar 31, 2024
1 parent cfd0c6b commit 8b7ba0d
Show file tree
Hide file tree
Showing 34 changed files with 234 additions and 314 deletions.
18 changes: 10 additions & 8 deletions openraft/src/core/notify.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use std::fmt;

use crate::core::sm;
use crate::raft::VoteResponse;
use crate::replication;
use crate::MessageSummary;
use crate::RaftTypeConfig;
use crate::Vote;

Expand Down Expand Up @@ -57,36 +58,37 @@ where C: RaftTypeConfig
}
}

impl<C> MessageSummary<Notify<C>> for Notify<C>
impl<C> fmt::Display for Notify<C>
where C: RaftTypeConfig
{
fn summary(&self) -> String {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::VoteResponse {
target,
resp,
sender_vote: vote,
} => {
format!("VoteResponse: from: {}: {}, res-vote: {}", target, resp.summary(), vote)
write!(f, "VoteResponse: from: {}: {}, res-vote: {}", target, resp, vote)
}
Self::HigherVote {
ref target,
higher: ref new_vote,
sender_vote: ref vote,
} => {
format!(
write!(
f,
"Seen a higher vote: target: {}, vote: {}, server_state_vote: {}",
target, new_vote, vote
)
}
Self::Network { response } => {
format!("Replication command done: {}", response.summary())
write!(f, "Replication command done: {}", response)
}
Self::StateMachine { command_result } => {
format!("StateMachine command done: {:?}", command_result)
write!(f, "StateMachine command done: {:?}", command_result)
}
Self::Tick { i } => {
format!("Tick {}", i)
write!(f, "Tick {}", i)
}
}
}
Expand Down
30 changes: 15 additions & 15 deletions openraft/src/core/raft_core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ use crate::core::sm::handle;
use crate::core::sm::CommandSeq;
use crate::core::ServerState;
use crate::display_ext::DisplayOption;
use crate::display_ext::DisplayOptionExt;
use crate::display_ext::DisplaySlice;
use crate::engine::Command;
use crate::engine::Condition;
Expand Down Expand Up @@ -91,7 +92,6 @@ use crate::ChangeMembers;
use crate::Instant;
use crate::LogId;
use crate::Membership;
use crate::MessageSummary;
use crate::OptionalSend;
use crate::RaftTypeConfig;
use crate::StorageError;
Expand Down Expand Up @@ -406,7 +406,7 @@ where
// request failures.

let _ = tx.send(Err(QuorumNotEnough {
cluster: eff_mem.membership().summary(),
cluster: eff_mem.membership().to_string(),
got: granted,
}
.into()));
Expand Down Expand Up @@ -593,7 +593,7 @@ where
false
});

tracing::debug!("report_metrics: {}", m.summary());
tracing::debug!("report_metrics: {}", m);
let res = self.tx_metrics.send(m);

if let Err(err) = res {
Expand Down Expand Up @@ -653,7 +653,7 @@ where
pub(crate) fn current_leader(&self) -> Option<C::NodeId> {
tracing::debug!(
self_id = display(self.id),
vote = display(self.engine.state.vote_ref().summary()),
vote = display(self.engine.state.vote_ref()),
"get current_leader"
);

Expand Down Expand Up @@ -1032,7 +1032,7 @@ where
}

/// Spawn parallel vote requests to all cluster members.
#[tracing::instrument(level = "trace", skip_all, fields(vote=vote_req.summary()))]
#[tracing::instrument(level = "trace", skip_all)]
async fn spawn_parallel_vote_requests(&mut self, vote_req: &VoteRequest<C>) {
let members = self.engine.state.membership_state.effective().voter_ids();

Expand Down Expand Up @@ -1097,7 +1097,7 @@ where

#[tracing::instrument(level = "debug", skip_all)]
pub(super) fn handle_vote_request(&mut self, req: VoteRequest<C>, tx: VoteTx<C>) {
tracing::info!(req = display(req.summary()), func = func_name!());
tracing::info!(req = display(&req), func = func_name!());

let resp = self.engine.handle_vote_req(req);
self.engine.output.push_command(Command::Respond {
Expand All @@ -1108,7 +1108,7 @@ where

#[tracing::instrument(level = "debug", skip_all)]
pub(super) fn handle_append_entries_request(&mut self, req: AppendEntriesRequest<C>, tx: AppendEntriesTx<C>) {
tracing::debug!(req = display(req.summary()), func = func_name!());
tracing::debug!(req = display(&req), func = func_name!());

let is_ok = self.engine.handle_append_entries(&req.vote, req.prev_log_id, req.entries, Some(tx));

Expand All @@ -1120,7 +1120,7 @@ where
// TODO: Make this method non-async. It does not need to run any async command in it.
#[tracing::instrument(level = "debug", skip(self, msg), fields(state = debug(self.engine.state.server_state), id=display(self.id)))]
pub(crate) async fn handle_api_msg(&mut self, msg: RaftMsg<C>) {
tracing::debug!("recv from rx_api: {}", msg.summary());
tracing::debug!("recv from rx_api: {}", msg);

match msg {
RaftMsg::AppendEntries { rpc, tx } => {
Expand All @@ -1130,7 +1130,7 @@ where
let now = InstantOf::<C>::now();
tracing::info!(
now = debug(now),
vote_request = display(rpc.summary()),
vote_request = display(&rpc),
"received RaftMsg::RequestVote: {}",
func_name!()
);
Expand Down Expand Up @@ -1206,7 +1206,7 @@ where
// TODO: Make this method non-async. It does not need to run any async command in it.
#[tracing::instrument(level = "debug", skip_all, fields(state = debug(self.engine.state.server_state), id=display(self.id)))]
pub(crate) fn handle_notify(&mut self, notify: Notify<C>) -> Result<(), Fatal<C>> {
tracing::debug!("recv from rx_notify: {}", notify.summary());
tracing::debug!("recv from rx_notify: {}", notify);

match notify {
Notify::VoteResponse {
Expand All @@ -1218,7 +1218,7 @@ where

tracing::info!(
now = debug(now),
resp = display(resp.summary()),
resp = display(&resp),
"received Notify::VoteResponse: {}",
func_name!()
);
Expand Down Expand Up @@ -1364,7 +1364,7 @@ where
sm::Response::BuildSnapshot(meta) => {
tracing::info!(
"sm::StateMachine command done: BuildSnapshot: {}: {}",
meta.summary(),
meta,
func_name!()
);

Expand All @@ -1380,7 +1380,7 @@ where
sm::Response::InstallSnapshot(meta) => {
tracing::info!(
"sm::StateMachine command done: InstallSnapshot: {}: {}",
meta.summary(),
meta.display(),
func_name!()
);

Expand Down Expand Up @@ -1526,8 +1526,8 @@ where
if &session_id.membership_log_id != self.engine.state.membership_state.effective().log_id() {
tracing::warn!(
"membership_log_id changed: msg sent by: {}; curr: {}; ignore when ({})",
session_id.membership_log_id.summary(),
self.engine.state.membership_state.effective().log_id().summary(),
session_id.membership_log_id.display(),
self.engine.state.membership_state.effective().log_id().display(),
msg
);
return false;
Expand Down
37 changes: 19 additions & 18 deletions openraft/src/core/raft_msg/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::collections::BTreeMap;
use std::fmt;

use crate::core::raft_msg::external_command::ExternalCommand;
use crate::error::CheckIsLeaderError;
Expand All @@ -16,7 +17,6 @@ use crate::type_config::alias::LogIdOf;
use crate::type_config::alias::OneshotSenderOf;
use crate::type_config::alias::SnapshotDataOf;
use crate::ChangeMembers;
use crate::MessageSummary;
use crate::RaftTypeConfig;
use crate::Snapshot;
use crate::Vote;
Expand Down Expand Up @@ -103,36 +103,37 @@ where C: RaftTypeConfig
},
}

impl<C> MessageSummary<RaftMsg<C>> for RaftMsg<C>
impl<C> fmt::Display for RaftMsg<C>
where C: RaftTypeConfig
{
fn summary(&self) -> String {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
RaftMsg::AppendEntries { rpc, .. } => {
format!("AppendEntries: {}", rpc.summary())
// TODO: avoid using summary()
write!(f, "AppendEntries: {}", rpc)
}
RaftMsg::RequestVote { rpc, .. } => {
format!("RequestVote: {}", rpc.summary())
write!(f, "RequestVote: {}", rpc)
}
RaftMsg::BeginReceivingSnapshot { .. } => {
write!(f, "BeginReceivingSnapshot")
}
RaftMsg::BeginReceivingSnapshot { .. } => "BeginReceivingSnapshot".to_string(),
RaftMsg::InstallFullSnapshot { vote, snapshot, .. } => {
format!("InstallFullSnapshot: vote: {}, snapshot: {}", vote, snapshot)
write!(f, "InstallFullSnapshot: vote: {}, snapshot: {}", vote, snapshot)
}
RaftMsg::ClientWriteRequest { .. } => "ClientWriteRequest".to_string(),
RaftMsg::CheckIsLeaderRequest { .. } => "CheckIsLeaderRequest".to_string(),
RaftMsg::ClientWriteRequest { .. } => write!(f, "ClientWriteRequest"),
RaftMsg::CheckIsLeaderRequest { .. } => write!(f, "CheckIsLeaderRequest"),
RaftMsg::Initialize { members, .. } => {
format!("Initialize: {:?}", members)
// TODO: avoid using Debug
write!(f, "Initialize: {:?}", members)
}
RaftMsg::ChangeMembership {
changes: members,
retain,
..
} => {
format!("ChangeMembership: members: {:?}, retain: {}", members, retain,)
RaftMsg::ChangeMembership { changes, retain, .. } => {
// TODO: avoid using Debug
write!(f, "ChangeMembership: members: {:?}, retain: {}", changes, retain,)
}
RaftMsg::ExternalCoreRequest { .. } => "External Request".to_string(),
RaftMsg::ExternalCoreRequest { .. } => write!(f, "External Request"),
RaftMsg::ExternalCommand { cmd } => {
format!("ExternalCommand: {:?}", cmd)
write!(f, "ExternalCommand: {}", cmd)
}
}
}
Expand Down
41 changes: 18 additions & 23 deletions openraft/src/engine/engine_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ use crate::raft::VoteRequest;
use crate::raft::VoteResponse;
use crate::raft_state::LogStateReader;
use crate::raft_state::RaftState;
use crate::summary::MessageSummary;
use crate::type_config::alias::InstantOf;
use crate::type_config::alias::SnapshotDataOf;
use crate::Instant;
Expand Down Expand Up @@ -168,7 +167,7 @@ where C: RaftTypeConfig
let m = entry.get_membership().expect("the only log entry for initializing has to be membership log");
self.check_members_contain_me(m)?;

tracing::debug!("update effective membership: log_id:{} {}", log_id, m.summary());
tracing::debug!("update effective membership: log_id:{} {}", log_id, m);

let em = EffectiveMembership::new_arc(Some(log_id), m.clone());
self.state.membership_state.append(em);
Expand Down Expand Up @@ -257,10 +256,10 @@ where C: RaftTypeConfig
// Make default vote-last-modified a low enough value, that expires leader lease.
let vote_utime = self.state.vote_last_modified().unwrap_or_else(|| now - lease - Duration::from_millis(1));

tracing::info!(req = display(req.summary()), "Engine::handle_vote_req");
tracing::info!(req = display(&req), "Engine::handle_vote_req");
tracing::info!(
my_vote = display(self.state.vote_ref().summary()),
my_last_log_id = display(self.state.last_log_id().summary()),
my_vote = display(self.state.vote_ref()),
my_last_log_id = display(self.state.last_log_id().display()),
"Engine::handle_vote_req"
);
tracing::info!(
Expand Down Expand Up @@ -298,8 +297,8 @@ where C: RaftTypeConfig
} else {
tracing::info!(
"reject vote-request: by last_log_id: !(req.last_log_id({}) >= my_last_log_id({})",
req.last_log_id.summary(),
self.state.last_log_id().summary(),
req.last_log_id.display(),
self.state.last_log_id().display(),
);
// The res is not used yet.
// let _res = Err(RejectVoteRequest::ByLastLogId(self.state.last_log_id().copied()));
Expand All @@ -316,11 +315,7 @@ where C: RaftTypeConfig

let res = self.vote_handler().update_vote(&req.vote);

tracing::info!(
req = display(req.summary()),
result = debug(&res),
"handle vote request result"
);
tracing::info!(req = display(&req), result = debug(&res), "handle vote request result");

let vote_granted = res.is_ok();

Expand All @@ -336,10 +331,10 @@ where C: RaftTypeConfig
#[tracing::instrument(level = "debug", skip(self, resp))]
pub(crate) fn handle_vote_resp(&mut self, target: C::NodeId, resp: VoteResponse<C>) {
tracing::info!(
resp = display(resp.summary()),
resp = display(&resp),
target = display(target),
my_vote = display(self.state.vote_ref()),
my_last_log_id = display(self.state.last_log_id().summary()),
my_last_log_id = display(self.state.last_log_id().display()),
"{}",
func_name!()
);
Expand Down Expand Up @@ -379,7 +374,7 @@ where C: RaftTypeConfig
// Seen a higher log. Record it so that the next election will be delayed for a while.
if resp.last_log_id.as_ref() > self.state.last_log_id() {
tracing::info!(
greater_log_id = display(resp.last_log_id.summary()),
greater_log_id = display(resp.last_log_id.display()),
"seen a greater log id when {}",
func_name!()
);
Expand All @@ -400,10 +395,10 @@ where C: RaftTypeConfig
) -> bool {
tracing::debug!(
vote = display(vote),
prev_log_id = display(prev_log_id.summary()),
prev_log_id = display(prev_log_id.display()),
entries = display(DisplaySlice::<_>(&entries)),
my_vote = display(self.state.vote_ref()),
my_last_log_id = display(self.state.last_log_id().summary()),
my_last_log_id = display(self.state.last_log_id().display()),
"{}",
func_name!()
);
Expand Down Expand Up @@ -442,9 +437,9 @@ where C: RaftTypeConfig
#[tracing::instrument(level = "debug", skip_all)]
pub(crate) fn handle_commit_entries(&mut self, leader_committed: Option<LogId<C::NodeId>>) {
tracing::debug!(
leader_committed = display(leader_committed.summary()),
my_accepted = display(self.state.accepted().summary()),
my_committed = display(self.state.committed().summary()),
leader_committed = display(leader_committed.display()),
my_accepted = display(self.state.accepted().display()),
my_committed = display(self.state.committed().display()),
"{}",
func_name!()
);
Expand Down Expand Up @@ -507,8 +502,8 @@ where C: RaftTypeConfig

tracing::debug!(
"membership: {}, committed: {}, is_leading: {}",
em.summary(),
self.state.committed().summary(),
em,
self.state.committed().display(),
self.state.is_leading(&self.config.id),
);

Expand Down Expand Up @@ -682,7 +677,7 @@ where C: RaftTypeConfig
}

tracing::error!(
last_log_id = display(self.state.last_log_id().summary()),
last_log_id = display(self.state.last_log_id().display()),
vote = display(self.state.vote_ref()),
"Can not initialize"
);
Expand Down
Loading

0 comments on commit 8b7ba0d

Please sign in to comment.