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

rkyv support #316

Open
djahandarie opened this issue Apr 25, 2022 · 5 comments · May be fixed by #879
Open

rkyv support #316

djahandarie opened this issue Apr 25, 2022 · 5 comments · May be fixed by #879

Comments

@djahandarie
Copy link

I'm trying to use openraft with 20k+ messages per second, and I seem to be running into quite a bit of overhead simply allocating/copying/serializing stuff. I'm currently using bincode, but would like to switch to rkyv as I believe it will do better.

Unfortunately rkyv doesn't work with serde Serialize/Deserialize (due to fundamental differences around supporting zero-copy), and thus there would need to be support added into openraft for me to be able to use rkyv to serialize things like VoteRequest/AppendEntriesRequest/InstallSnapshotRequest.

I think it would just be a matter of adding some different derives behind a feature flag for rkyv. Is there interest in this? If so I will try to make a PR.

@github-actions
Copy link

👋 Thanks for opening this issue!

Get help or engage by:

  • /help : to print help messages.
  • /assignme : to assign this issue to you.

@drmingdrmer
Copy link
Member

There is an ongoing PR to make serde an optional feature:

What you need looks similar to this? An optional feature that enables several rkyv derive?

I do not have any experience on rkyv other than reading its API doc:)
A PR is welcomed! :DDD

@MikaelCall
Copy link

Adding rkyv support would be a nice addition indeed. It's a very efficient format wrt (de)serialization speed which can easily become a bottleneck.

IMHO the solution outlined in the comment #243 (review) in the currently open serde PR would be a very simple and good solution wrt maintainablity.

@zach-schoenberger
Copy link
Contributor

zach-schoenberger commented Jul 25, 2022

Hi All, I'm looking at adding this feature and ran into a small issue. Is there any issue with moving away from using AnyError directly in some of the messages? An example:

#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize), serde(bound = ""))]
#[error("NetworkError: {source}")]
pub struct NetworkError {
#[from]
source: AnyError,
}

The issue being that AnyError is defined in the external crate so we can't easily add the rkyv traits to it

(Edit)
I see now that the AnyError crate is actually apart of drmindrmer's repositories. I guess my question becomes if rkyv support should be added there?

(Edit 2)
Looking more closely at this, it doesn't appear that the error's themselves would/should ever be sent over the network so it doesn't look like there would be any reason to add this serialization to them. I should have a pr for this sometime today.

@drmingdrmer
Copy link
Member

Hi All, I'm looking at adding this feature and ran into a small issue. Is there any issue with moving away from using AnyError directly in some of the messages? An example:

#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize), serde(bound = ""))]
#[error("NetworkError: {source}")]
pub struct NetworkError {
#[from]
source: AnyError,
}

The issue being that AnyError is defined in the external crate so we can't easily add the rkyv traits to it

(Edit) I see now that the AnyError crate is actually apart of drmindrmer's repositories. I guess my question becomes if rkyv support should be added there?

(Edit 2) Looking more closely at this, it doesn't appear that the error's themselves would/should ever be sent over the network so it doesn't look like there would be any reason to add this serialization to them. I should have a pr for this sometime today.

It is possible to add rkyv to AnyError.

But AFAIK, only types that need to be persisted on disk need rkyv support.
Errors in openraft are only used for transport.
Does it have to be encoded in the rkyv format for transport?
E.g., for `send_append_entries():

async fn send_append_entries(
&mut self,
rpc: AppendEntriesRequest<C>,
) -> Result<AppendEntriesResponse<C::NodeId>, RPCError<C::NodeId, AppendEntriesError<C::NodeId>>>;

The response and response errors are small structs, maybe it's possible to use some other serialization such as serde to encode responses?

@df-oss df-oss linked a pull request Jun 27, 2023 that will close this issue
3 tasks
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 a pull request may close this issue.

4 participants