Conversation
drmingdrmer
left a comment
There was a problem hiding this comment.
Thanks for contribution for the rkyv feature!
Usually I do not accept a totally AI generated patch. Meanwhile, even with the most advanced model, AI generated patch very likely is not qualified at production level.
There are several issues with this PR:
- rkyv feature is not actually used in the added example.
- lacking test to cover the added rkyv features.
- not all of the types need rkyv support.
The following is AI generated analysis about what types need rkyv derive:
Serialization Feature Flag Design
Background
The current codebase has a single serde feature flag that gates serde::Serialize/serde::Deserialize
derives on all types uniformly. The new rkyv feature flag was added the same way —
applied to every type that had serde.
But not every type serves the same purpose. Openraft types fall into distinct categories
with different serialization needs:
-
Storage types: persisted to log store / snapshot store.
Need both serde (for JSON/bincode storage backends) and rkyv (for zero-copy storage backends). -
Transport types: sent between Raft nodes over the network.
Need both serde (for JSON/protobuf/bincode transport) and rkyv (for zero-copy network transport). -
Error types: returned from API calls and RPCs, never persisted or sent as raw bytes.
Need serde (for JSON error responses, logging), but NOT rkyv. -
Config/Runtime types: runtime configuration, metrics, ephemeral state.
Need serde (for config files, metrics export), but NOT rkyv.
The serde and rkyv feature flags should be split along these boundaries:
| Feature flag | Storage | Transport | Error | Config/Runtime |
|---|---|---|---|---|
serde (current) |
yes | yes | yes | yes |
Proposed: serde-storage |
yes | - | - | - |
Proposed: serde-transport |
- | yes | - | - |
Proposed: serde-error |
- | - | yes | - |
Proposed: serde-config |
- | - | - | yes |
rkyv |
yes | yes | no | no |
The current serde flag could remain as a convenience that enables all serde sub-features.
The rkyv flag only applies to storage + transport types — error and config types never need rkyv.
Chapter 1: Types That Need rkyv
1.1 Storage Types
Persisted to log storage, snapshot, or state machine.
These are types appearing in RaftLogStorage / RaftStateMachine trait signatures
and their transitive field dependencies.
| Type | File | Reason |
|---|---|---|
LogId<C> |
log_id/mod.rs |
Core storage key, every log entry has one |
LeaderId<C> (adv) |
vote/leader_id/leader_id_adv.rs |
Field inside LogId |
LeaderId<C> (std) |
vote/leader_id/leader_id_std.rs |
Field inside LogId |
CommittedLeaderId<C> |
vote/leader_id/leader_id_std.rs |
Field inside LogId (std mode) |
Vote<C> |
vote/vote.rs |
Persisted by save_vote() |
Entry<C> |
entry/entry.rs |
The log entry itself |
EntryPayload<C> |
entry/payload.rs |
Field of Entry |
Membership<C> |
membership/membership.rs |
Stored inside entries and StoredMembership |
StoredMembership<C> |
membership/stored_membership.rs |
Returned by applied_state(), stored in snapshot |
SnapshotMeta<C> |
storage/snapshot_meta.rs |
Snapshot metadata, persisted alongside snapshot data |
SnapshotSignature<C> |
storage/snapshot_signature.rs |
Derived from SnapshotMeta, identifies snapshot |
EmptyNode |
node.rs |
Node type stored inside Membership |
BasicNode |
node.rs |
Node type stored inside Membership |
1.2 Transport/RPC Types
Network messages sent between Raft nodes via RaftNetworkV2 trait.
rkyv enables zero-copy deserialization on the hot path (AppendEntries).
| Type | File | Notes |
|---|---|---|
VoteRequest<C> |
raft/message/vote.rs |
Election RPC |
VoteResponse<C> |
raft/message/vote.rs |
Election reply |
AppendEntriesRequest<C> |
raft/message/append_entries_request.rs |
Hot path. Replication RPC. Contains Vec<Entry> |
AppendEntriesResponse<C> |
raft/message/append_entries_response.rs |
Replication reply |
InstallSnapshotRequest<C> |
raft/message/install_snapshot.rs |
Snapshot transfer. Contains SnapshotMeta, Vec<u8> |
InstallSnapshotResponse<C> |
raft/message/install_snapshot.rs |
Snapshot reply |
SnapshotResponse<C> |
raft/message/install_snapshot.rs |
Alternative snapshot response |
TransferLeaderRequest<C> |
raft/message/transfer_leader.rs |
Leadership transfer |
StreamAppendError<C> |
raft/message/stream_append_error.rs |
Sent back over the wire during streaming |
ClientWriteResponse<C> |
raft/message/client_write.rs |
Returned to clients; could be forwarded between nodes |
WriteResponse<C> |
raft/message/write.rs |
Simplified version of ClientWriteResponse |
SnapshotSegmentId |
raft_types.rs |
Used in snapshot streaming protocol |
Chapter 2: Types That Have rkyv But Should Not
These types were given #[cfg_attr(feature = "rkyv", derive(rkyv::Archive, rkyv::Deserialize, rkyv::Serialize))]
in commit addce54c but do not need it. They should have the rkyv derive removed.
Similarly, the serde derive on these types could be gated under more specific feature flags
(e.g., serde-error, serde-config) instead of the blanket serde flag,
so users who only need storage/transport serialization don't pull in serde impls for 30+ error types.
2.1 Error Types
Never persisted or sent as rkyv-serialized data.
Errors flow through local API calls and are serialized (when needed) as JSON for logging or HTTP responses.
Some errors appear as enum variants inside transport response types
(e.g., HigherVote is a variant of AppendEntriesResponse),
but those are part of the response type's own rkyv derive — the standalone error types are not serialized separately.
Action: remove rkyv derive. Consider gating serde under serde-error or similar.
| Type | File |
|---|---|
Fatal<C> |
errors/fatal.rs |
StorageError<C> |
errors/storage_error.rs |
ErrorSubject<C> |
errors/storage_error.rs |
ErrorVerb |
errors/storage_error.rs |
RaftError<C, E> |
errors/raft_error.rs |
ClientWriteError<C> |
errors/mod.rs |
ChangeMembershipError<C> |
errors/mod.rs |
InitializeError<C> |
errors/mod.rs |
RPCError<C, E> |
errors/mod.rs |
RemoteError<C, T> |
errors/mod.rs |
NetworkError<C> |
errors/mod.rs |
Unreachable<C> |
errors/mod.rs |
Timeout<C> |
errors/mod.rs |
ForwardToLeader<C> |
errors/mod.rs |
ConflictingLogId<C> |
errors/conflicting_log_id.rs |
HigherVote<C> |
errors/higher_vote.rs |
RejectVote<C> |
errors/reject_vote.rs |
LinearizableReadError<C> |
errors/linearizable_read_error.rs |
AllowNextRevertError<C> |
errors/allow_next_revert_error.rs |
MembershipError<C> |
errors/membership_error.rs |
NodeNotFound<C> |
errors/node_not_found.rs |
ReplicationClosed |
errors/replication_closed.rs |
StreamingError<C> |
errors/streaming_error.rs |
InstallSnapshotError |
errors/mod.rs |
SnapshotMismatch |
errors/mod.rs |
QuorumNotEnough<C> |
errors/mod.rs |
InProgress<C> |
errors/mod.rs |
LearnerNotFound<C> |
errors/mod.rs |
NotAllowed<C> |
errors/mod.rs |
NotInMembers<C> |
errors/mod.rs |
EmptyMembership |
errors/mod.rs |
Operation |
errors/operation.rs |
NoForward |
errors/mod.rs |
Infallible |
errors/mod.rs |
BoxedErrorSource |
impls/boxed_error_source.rs |
2.2 Config/Runtime/API-Input Types
Runtime state — never persisted as raft data, never sent over RPC as raw bytes.
Action: remove rkyv derive. Consider gating serde under serde-config or similar.
| Type | File | Why no rkyv |
|---|---|---|
Config |
config/config.rs |
Runtime configuration, loaded from file/env |
SnapshotPolicy |
config/config.rs |
Field of Config |
RaftMetrics<C> |
metrics/raft_metrics.rs |
Observable runtime metrics, never stored |
RaftDataMetrics<C> |
metrics/raft_metrics.rs |
Metrics subset |
RaftServerMetrics<C> |
metrics/raft_metrics.rs |
Metrics subset |
ServerState |
core/server_state.rs |
Ephemeral runtime state (Leader/Follower/etc.) |
ChangeMembers<C> |
change_members.rs |
API input to change_membership() |
WaitError |
metrics/wait.rs |
Metrics wait error |
2.3 Internal/Utility Types
These are pub(crate) or serde-specific utilities. Not part of any public storage or transport interface.
Action: remove rkyv derive.
| Type | File | Why no rkyv |
|---|---|---|
CommittedVote<C> |
vote/committed.rs |
pub(crate) internal wrapper. Not in any storage/transport trait signature. |
UncommittedVote<C> |
vote/non_committed.rs |
pub(crate) internal wrapper. |
Joint<...> |
quorum/joint.rs |
pub(crate). Not a serialized field of Membership — constructed at runtime from configs: Vec<BTreeSet<NodeId>>. |
Compat<From, To> |
compat/upgrade.rs |
Serde-specific migration helper (#[serde(untagged)]). rkyv has a different archive format; this type is meaningless for rkyv. |
RPCTypes |
network/rpc_type.rs |
Enum naming RPC types. Used in error context fields, not serialized on the wire. |
SerdeInstant |
metrics/serde_instant.rs |
Only used in RaftMetrics. No stored or transport type contains it. Manual rkyv impl can be kept as convenience but is not required. |
2.4 Test-Only Types
Action: remove rkyv derive.
| Type | File |
|---|---|
UTConfig<N> |
engine/testing.rs |
TickUTConfig |
core/tick.rs |
TestNode |
node.rs |
Summary
| Category | Count | rkyv | serde flag |
|---|---|---|---|
| Storage types | 13 | yes | serde-storage |
| Transport/RPC types | 12 | yes | serde-transport |
| Error types | 34 | no | serde-error |
| Config/Runtime/API | 8 | no | serde-config |
| Internal/Utility | 6 | no | varies |
| Test-only | 3 | no | n/a |
Of the ~70 types that received rkyv derives in commit addce54c, ~25 need it (storage + transport).
The remaining ~45 should have rkyv removed (errors, config, runtime, internal, test).
The existing serde feature flag can be kept as a convenience alias that enables all sub-features.
@drmingdrmer reviewed 38 files and all commit messages, and made 2 comments.
Reviewable status: 38 of 78 files reviewed, 1 unresolved discussion (waiting on Nicholas-Ball).
examples/types-kv-rkyv/src/lib.rs line 52 at r1 (raw file):
D = Request, R = Response, );
types-kv is a crate contains request/response struct definition. to support rkyv, adding rkyv derive attributes to crate types-kv would be better, than creating a duplicated crate.
Unless there is something I missed that prohibits using rkyv in types-kv.
And this declared TypeConfig does not seem to be used anywhere.
Code quote:
openraft::declare_raft_types!(
pub TypeConfig:
D = Request,
R = Response,
);|
I see. Would you like me to add a rkyv-storage and a rkyv-transport feature flag with a rkyv feature that enables both or do you want me just to do the rkyv feature for now |
just rkyv-storage looks good. I am not very sure if rkyv-transport would be necessary, since rkyv is not designed for transport. and AFAIK there are a few compatibility issue with transmitting rkyv messages. |
I know this was has been a feature that has been attempted a few times so I am hoping this finally pushes it across the metaphoric finish line.
This approach is very similar to #879 to resolve #316. There are a few differences. The first is rkyv no longer uses archive(check_bytes). They now use bytecheck which is implemented automatically for structs that implement the rkyv::Archive attribute when bytecheck feature is enabled. Bytecheck feature is enabled by default.
Rkyv doesn't like enums without variants, so I manually implemented rkyv for NoForward and Infallible.
I used the serde feature as a guide of what needs to also support rkyv so it is possible I implemented rkyv on something that doesn't need it by accident but I am pretty certain and have doubled check that I didn't get anything included by accident.
I added tests and examples of how to use rkyv with openraft. These tests and examples are mostly 1-1 copies of the existing serde tests with serde replaced.
I also updated the docs to include the new feature.
Let me know if there is anything I missed.
Checklist
This change is