Feat: Make Batch a customizable type in RaftTypeConfig#1657
Feat: Make Batch a customizable type in RaftTypeConfig#1657Lymah123 wants to merge 1 commit intodatabendlabs:mainfrom
Conversation
66f46be to
7adb711
Compare
7adb711 to
97a396e
Compare
|
Hi @drmingdrmer and @schreter . This PR is ready for review. |
drmingdrmer
left a comment
There was a problem hiding this comment.
Any AI generated codes should be carefully reviewed before being pushed to github.
@drmingdrmer reviewed 19 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Lymah123).
openraft/src/base/batch/display.rs line 37 at r3 (raw file):
where T: fmt::Display + OptionalSend + 'static + fmt::Debug, B: RaftBatch<T>,
T already is Display. it does not seem it needs Debug.
Code quote:
impl<'a, T, B> fmt::Display for DisplayBatch<'a, T, B>
where
T: fmt::Display + OptionalSend + 'static + fmt::Debug,
B: RaftBatch<T>,openraft/src/base/batch/display.rs line 70 at r3 (raw file):
assert_eq!( format!("{}", <Batch<i32> as RaftBatch<i32>>::display(&Batch::Single(42))), "[42]"
Doesn't Batch::Single(42i32).display() work here?
Code quote:
format!("{}", <Batch<i32> as RaftBatch<i32>>::display(&Batch::Single(42))),
"[42]"openraft/src/base/batch/raft_batch.rs line 26 at r3 (raw file):
/// - Consuming iteration via `into_iter()` method with explicit `IntoIter` associated type. pub trait RaftBatch<T>: OptionalSend + Sized + Default + Debug + 'static where T: OptionalSend + Debug + 'static
I'm not sure if all of the methods this trait provides are used in our codebase. If not, please consider removing the unused type of method.
Code quote:
pub trait RaftBatch<T>: OptionalSend + Sized + Default + Debug + 'static
where T: OptionalSend + Debug + 'staticopenraft/src/base/batch/mod.rs line 168 at r3 (raw file):
_ => Batch::Vec(iter.collect()), } }
If it already provided from_exact_iter(), then from_item() and from_vec can just provide a default implementation that calls from_exact_iter(), and user does not need to implement these two methods.
Code quote:
fn from_item(item: T) -> Self {
Batch::Single(item)
}
fn from_vec(vec: Vec<T>) -> Self {
Batch::from(vec)
}
fn from_exact_iter<I>(iter: I) -> Self
where I: ExactSizeIterator<Item = T> {
match iter.len() {
0 => Batch::Vec(Vec::new()),
1 => Batch::Single(iter.into_iter().next().unwrap()),
_ => Batch::Vec(iter.collect()),
}
}openraft/src/engine/command.rs line 234 at r3 (raw file):
C: RaftTypeConfig, C::Entry: PartialEq, C::Batch<C::Entry>: PartialEq,
We should add PartialEq to the RaftBatch trait. since PartialEq is a non-optional requirement here.
openraft/src/base/batch/iter.rs line 48 at r3 (raw file):
// Safety: BatchIter<T> is Send when T is Send because both Option<T> and vec::IntoIter<T> are Send unsafe impl<T: Send> Send for BatchIter<T> {}
Usually, there is no need to manually impl Send for a type in safe rust.
What's the necessity of this?
This looks like AI generated code without explicit reason.
Code quote:
// Safety: BatchIter<T> is Send when T is Send because both Option<T> and vec::IntoIter<T> are Send
unsafe impl<T: Send> Send for BatchIter<T> {}There was a problem hiding this comment.
Pull request overview
This PR introduces a customizable batch container abstraction for Openraft by adding a RaftBatch trait and a new RaftTypeConfig::Batch<T> associated type, then wiring internal message/engine paths to use C::Batch<_> instead of the fixed internal Batch<T>.
Changes:
- Add
RaftBatchtrait and make the defaultBatch<T>public + implementRaftBatchfor it. - Extend
RaftTypeConfigwithtype Batch<T>(defaulted indeclare_raft_types!) and re-export the default implementation viacrate::impls::Batch. - Update core/engine/API code paths to construct/consume batches through
C::Batchand adjust responder debug behavior.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| openraft/src/type_config.rs | Adds RaftTypeConfig::Batch<T> and an alias for it; documents how to customize. |
| openraft/src/raft/responder/core_responder.rs | Adds a manual Debug impl to satisfy new batch element bounds without exposing responder internals. |
| openraft/src/raft/mod.rs | Updates declare_raft_types! defaults to include Batch<T> = impls::Batch<T>. |
| openraft/src/raft/message/write_request.rs | Switches client write request construction to C::Batch::from_item(...). |
| openraft/src/raft/api/app.rs | Updates client write APIs to accept/build C::Batch<_> payloads and responders. |
| openraft/src/lib.rs | Re-exports RaftBatch from the crate root. |
| openraft/src/impls/mod.rs | Re-exports the default Batch implementation from base::batch. |
| openraft/src/engine/handler/leader_handler/mod.rs | Builds entry batches via C::Batch::from_exact_iter(...). |
| openraft/src/engine/handler/following_handler/mod.rs | Converts appended entry vecs into C::Batch via from_vec. |
| openraft/src/engine/command_scheduler.rs | Uses RaftBatch APIs (len, extend) when merging append-entry commands. |
| openraft/src/engine/command.rs | Changes AppendEntries command entries field to C::Batch<C::Entry> and updates equality bounds. |
| openraft/src/core/raft_msg/mod.rs | Updates RaftMsg::ClientWrite fields to C::Batch<_>. |
| openraft/src/core/raft_core.rs | Adapts core write/append paths to consume batches via iterators and RaftBatch helpers. |
| openraft/src/core/merged_raft_msg_receiver.rs | Merges ClientWrite messages by extending C::Batch and updates tests to use the trait API. |
| openraft/src/base/mod.rs | Exposes RaftBatch from base (replacing the previous internal Batch re-export). |
| openraft/src/base/batch/raft_batch.rs | New: defines the RaftBatch trait API for customizable batching. |
| openraft/src/base/batch/mod.rs | Makes Batch<T> public and implements RaftBatch for it. |
| openraft/src/base/batch/iter.rs | Adds a Send impl for BatchIter<T> and keeps iterator behavior. |
| openraft/src/base/batch/display.rs | Generalizes display formatting to work with any RaftBatch implementation and updates tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// pub MyTypeConfig: | ||
| /// D = String, | ||
| /// R = String, | ||
| /// Batch<T> = MyCustomBatch<T> where T: OptionalSend + 'static, |
There was a problem hiding this comment.
The custom batch example’s where clause (where T: OptionalSend + 'static) no longer matches the actual Batch<T> requirement, which currently also requires T: Debug. Updating the example to include the Debug bound will prevent users from copying an example that doesn’t compile.
| /// Batch<T> = MyCustomBatch<T> where T: OptionalSend + 'static, | |
| /// Batch<T> = MyCustomBatch<T> where T: OptionalSend + 'static + Debug, |
| // Safety: BatchIter<T> is Send when T is Send because both Option<T> and vec::IntoIter<T> are Send | ||
| unsafe impl<T: Send> Send for BatchIter<T> {} | ||
|
|
There was a problem hiding this comment.
unsafe impl Send for BatchIter<T> appears unnecessary because BatchIter<T> is composed of Option<T> and vec::IntoIter<T>, which are already Send when T: Send, so the enum should auto-implement Send. Keeping an explicit unsafe impl is risky because it can silently become unsound if BatchIter internals change; prefer relying on auto traits (or, if needed, add safe bounds via wrapper types rather than unsafe).
| // Safety: BatchIter<T> is Send when T is Send because both Option<T> and vec::IntoIter<T> are Send | |
| unsafe impl<T: Send> Send for BatchIter<T> {} |
| fn iter_mut(&mut self) -> Self::IterMut<'_>; | ||
|
|
||
| /// Consumes the batch and returns an iterator over the elements. | ||
| fn into_iter(self) -> Self::IntoIter; | ||
|
|
There was a problem hiding this comment.
RaftBatch defines a custom into_iter(self) method while many batch implementations (including the default Batch<T>) also implement IntoIterator. This creates an API footgun (and can lead to method-call ambiguity in non-generic code when both traits are in scope). Consider making RaftBatch extend IntoIterator<Item = T, IntoIter = Self::IntoIter> and removing/renaming this method to avoid conflicts.
| /// guarantees. | ||
| /// - Consuming iteration via `into_iter()` method with explicit `IntoIter` associated type. | ||
| pub trait RaftBatch<T>: OptionalSend + Sized + Default + Debug + 'static | ||
| where T: OptionalSend + Debug + 'static |
There was a problem hiding this comment.
RaftBatch requires T: Debug via the trait-level where clause, but none of the trait methods require T: Debug (implementations can provide Debug for the batch without exposing element details, as done for CoreResponder). This extra bound reduces flexibility for custom batch element types; consider dropping T: Debug from RaftBatch (and then from RaftTypeConfig::Batch<T> / macro defaults) unless there is a concrete internal requirement.
| where T: OptionalSend + Debug + 'static | |
| where T: OptionalSend + 'static |
schreter
left a comment
There was a problem hiding this comment.
@schreter reviewed 19 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @drmingdrmer and @Lymah123).
openraft/src/base/batch/mod.rs line 147 at r3 (raw file):
type Iter<'a> = slice::Iter<'a, T> where T: 'a;
BTW, the code formatting looks odd in multiple places. Did you run rustfmt?
Code quote:
type Iter<'a>
= slice::Iter<'a, T>
where T: 'a;openraft/src/engine/handler/following_handler/mod.rs line 157 at r3 (raw file):
// A follower should always use the node's vote. committed_vote: self.leader_vote.clone(), entries: C::Batch::from_vec(entries),
Wouldn't it make sense to rework the method signature to receive a Batch directly instead of receiving a Vec? I assume, the caller could produce a Batch itself.
openraft/src/raft/api/app.rs line 129 at r3 (raw file):
} self.do_client_write_ff(C::Batch::from_vec(payloads), C::Batch::from_vec(responders)).await?;
Here, payloads is already an iterator. Why not remove the collect() above and construct the batch from iterator zipped with responders?
openraft/src/type_config.rs line 153 at r3 (raw file):
/// Providing a custom batch implementation: /// /// ```ignore
Maybe use no_run instead of ignore to at least compile the example code? (also above)
See the comment from the bot below.
openraft/src/raft/responder/core_responder.rs line 25 at r3 (raw file):
match self { Self::Progress(_) => f.debug_tuple("Progress").field(&"<responder>").finish(), Self::UserDefined(_) => f.debug_tuple("UserDefined").field(&"<responder>").finish(),
This is basically a fancy way for writing a string constant :-).
Suggestion:
Self::Progress(_) => f.write_str("Progress(<responder>)"),
Self::UserDefined(_) => f.write_str("UserDefined(<responder>)"),|
Thanks for the feedback. I will implement the suggested changes. |
Closes #1625
Checklist
This change is