Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: add rkyv support #879

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Feature: add rkyv support #879

wants to merge 1 commit into from

Conversation

df-oss
Copy link

@df-oss df-oss commented Jun 27, 2023

Checklist

  • Updated guide with pertinent info (may not always apply).
  • Squash down commits to one or two logical commits which clearly describe the work you've done.
  • Unittest is a friend:)

This change is Reviewable

@drmingdrmer
Copy link
Member

@df-oss

The tests for rkyv includes:

  • a storage implementation, e.g., another store in openraft/stores/, that ensures storage layer works as expected;
    The storage implementation should pass all tests defined in openraft::testing::suite:

    pub fn test_rocks_store() -> Result<(), StorageError<RocksNodeId>> {
      Suite::test_all(RocksBuilder {})?;
      Ok(())
    }
  • and an example app that ensures network implementation works as expected, similar to examples/raft-kv-memstore.

https://github.com/datafuselabs/openraft/blob/79588014ab9effc1ac8da5dafbdcef4c51283f4c/examples/raft-kv-memstore/tests/cluster/test_cluster.rs#L171-L177

@drmingdrmer
Copy link
Member

PR summary(by Claude):

Accept changes to Cargo.toml and enable rkyv serialization / deserialization support on schema types

@df-oss
Copy link
Author

df-oss commented Jul 16, 2023 via email

@drmingdrmer
Copy link
Member

  • RaftMetrics running_state field uses Fatal which implies that several errors need to implement the rkyv traits which includes the following two types:
    rkyv traits are not implemented for core::ops::range. I've created an issue rkyv/rkyv#404.

Where is core::ops::range used in error types? I can not remember. Please give a link to the code where it is used.

IMHO, errors and RaftMetrics can be serialized in another way other than rkyv. Why do you have to serialize them in rkyv?

Maybe the tracking issue #880
should be updated to mention/include rkyv support for transport and/or
storage and not just storage.

Agree. These three pairs of RPC struct also needs rkyv support:

  • AppendEntriesRequest
  • AppendEntriesResponse
  • VoteRequest
  • VoteResponse
  • InstallSnapshotRequest
  • InstallSnapshotResponse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rkyv support
2 participants