Improve: Make serde optional, a dirty work

[PsiACE]

Signed-off-by: Chojan Shang [email protected]

This is a simple implementation and serde will be the default feature. But of course, you can turn it off now with 'default-features = false'.

I need some suggestions to see how to make it better.

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. If you don't, then Dodd will :).

---

This change is 

[PsiACE]

A part of the code has been rewritten, it just works, but not elegantly enough.

[schreter]

LGTM

Reviewed 18 of 18 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @PsiACE)

---

openraft/src/lib.rs, line 89 at r1 (raw file):

Previously, PsiACE wrote…

A part of the code has been rewritten, it just works, but not elegantly enough.

Well, what you could do here is to define a supertrait RaftSerializable, which would be empty for non-serde implementation and implemented for any T and use Serialize + Deserialize supertraits for serde implementation and implemented for any T: Serialize + Deserialize. Then, make it supertrait of AppData and AppDataResponse and other types, as needed. It will reduce the amount of changes somewhat should the requirements change or another serialization should be supported.

But I think the current approach is good enough :-).

---

openraft/src/metrics.rs, line 31 at r2 (raw file):

/// A set of metrics describing the current state of a Raft node.
#[derive(Clone, Debug, PartialEq, Eq)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]

The derives could be possibly simplified with an own derive macro so you don't have to type so much and also to possibly support other serialization frameworks in the future by changing it in one place, but I think that's an overkill for the first version and this is good enough.

[schreter]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @PsiACE)

---

openraft/Cargo.toml, line 45 at r2 (raw file):

[features]
default = ["serde"]

What is missing, though, is a test crate which uses openraft without the default feature to verify that it won't get broken.

Not sure what is the best approach here, since so far there are no extra test crates yet. Probably add a binary test crate test_no_serde or so in the workspace, which won't get published to crates.io, just used in the build of the workspace to ensure compatibility.

NOTE: just adding a test won't help, since the features are cumulative and cargo would activate serde for the test. You indeed need a crate which doesn't have other dependencies and just uses openraft w/o default features. Then you can test that the types don't implement serde::Serialize by using something which doesn't use serde as AppData, for instance. You don't need to spin up Raft, just do the compile check there (i.e., one-liner main() which tries to convert a reference to some non-serde type to &dyn AppData).

[PsiACE]

hi @schreter , thank you for the kind review. I think we can disable serde by default. Then use serde_test to do some tests.

What do you think?

[schreter]

I think we can disable serde by default. Then use serde_test to do some tests.

Of course, not using serde by default and only activating it if the serde feature is on is the other (possibly preferable) option, but I'm not sure what is the best. That should be decided by one of the maintainers.

Independent of that, you still need to create an extra crate to test it with the feature enabled, then. It is to my knowledge simply impossible to test both with a feature enabled and a feature disabled inside of the same crate.

@drmingdrmer Opinions/directions?

[drmingdrmer]

@schreter @PsiACE
Disabling serde by default looks good to me.

Independent of that, you still need to create an extra crate to test it with the feature enabled, then. It is to my knowledge simply impossible to test both with a feature enabled and a feature disabled inside of the same crate.

I'm used to writing feature flag related unit tests like this:

#[cfg(feature = "backtrace")]
#[test]
fn test_backtrace() -> anyhow::Result<()> {
    let got = backtrace_str().expect("no none");
    assert!(got.contains("test_backtrace"));
    Ok(())
}

#[cfg(not(feature = "backtrace"))]
#[test]
fn test_backtrace() -> anyhow::Result<()> {
    let got = backtrace_str();
    assert!(got.is_none());
    Ok(())
}

And let CI run it twice: with such a feature flag on and off.

Good enough?

[schreter]

Good enough?

Sure, it just then takes double resources in the CI :-). But if there are sufficient resources, then this indeed is the best approach.

[PsiACE]

Good enough?

I've written this a lot in the past, and I think it's totally feasible.

[PsiACE]

A feature called "serde_impl" is currently used to control serde support. I haven't added any relevant tests for now, because I'm not sure which ones are necessary.

Since memstore and example-raft-kv both rely on openraft/serde_impl, I think they have helped us with a simple verification. Please remind me if I've missed anything.

[drmingdrmer]

memstore and example-raft-kv may not covers every types.
We need to ensure at least these types to be serializable(top-level types) in unit test(openraft unitest should be run twice by github workflow CI: with and without feature serde-impl):

• Essential types:
  Entry
RaftMetrics
Node

• Trait implementations:
  SomeType that impl NodeId
  SomeType that impl AppData
  SomeType that impl AppDataResponse

• User API errors:
  InitializeError
AddLearnerError
ChangeMembershipError
ClientWriteError
CheckIsLeaderError
Fatal

• Types used by Raft protocol API:
  RPCError.
  Request, Response and Errors that are used in RaftNetwork.

---

Edit:
The types used in Raft should also be included:
AddLearnerResponse
ClientWriteResponse

[drmingdrmer]

Update<T> does not need to be serializable.
It must have been added by accident. :(

[drmingdrmer]

Edit:
The types used in Raft should also be included:
AddLearnerResponse
ClientWriteResponse

Add two missing serializble types