Feature: monoio runtime support

[SteveLauC]

What does this PR do

This PR adds a new crate openraft-rt-monoio, which provides a type MonoioRuntime that has AsyncRuntime implemented to enable users to run Openraft on Monoio.

PR remains in draft stage until we have runtime test suite implemented and have this crate tested.

Some details

1. This new crate is called openraft-rt-monoio, while for the directory name, we simply use rt-monoio as it lives in the Openraft repo.

2. Crate version

   It uses the same version number as other crates:

   version       = { workspace = true }

3. The primitive wrapper types are put in a private module so that they are not exposed to the users, I did this because this approach is how we handle those the primitives used with TokioRuntime. However, this is indeed one exception, TokioInstant was re-exported to the users.

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 

[SteveLauC]

I am not sure about the root cause of that lint CI failure, I guess it fails because rt-monoio needs to enable the singlethreaded feature, which makes it incompatible with other crates?

[schreter]

I am not sure about the root cause of that lint CI failure, I guess it fails because rt-monoio needs to enable the singlethreaded feature, which makes it incompatible with other crates?

Yes, monoio is !Sync.

[drmingdrmer]

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

---

rt-monoio/src/lib.rs line 109 at r1 (raw file):

    #[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd)]
    pub struct MonoioInstant(pub(crate) monoio::time::Instant);

It looks like derive_more can be used to simplify some of these Add/AddAssign implementation:

https://jeltef.github.io/derive_more/derive_more/add.html#tuple-structs
https://jeltef.github.io/derive_more/derive_more/add_assign.html

[SteveLauC]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @drmingdrmer)

---

rt-monoio/src/lib.rs line 109 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

It looks like derive_more can be used to simplify some of these Add/AddAssign implementation:

https://jeltef.github.io/derive_more/derive_more/add.html#tuple-structs
https://jeltef.github.io/derive_more/derive_more/add_assign.html

Looks like the generated Add (and AddAssign) impl will set the Rhs argument to Self, i.e., MonoioInstant, we need it to be Duration, and it seems that there is no way to change this bahavior to satisfy our needs. 😶

[drmingdrmer]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SteveLauC)

---

rt-monoio/src/lib.rs line 109 at r1 (raw file):

Previously, SteveLauC (SteveLauC) wrote…

Looks like the generated Add (and AddAssign) impl will set the Rhs argument to Self, i.e., MonoioInstant, we need it to be Duration, and it seems that there is no way to change this bahavior to satisfy our needs. 😶

Right...

[schreter]

OK, but see the comment.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SteveLauC)

---

rt-monoio/Cargo.toml line 17 at r1 (raw file):

[dependencies]
openraft = { path= "../openraft", version = "0.10.0", features = ["singlethreaded"] }

BTW, can monoio runtime be part of the same workspace?

The issue I see here is that Cargo unifies features of all dependent crates. Thus, all crates in the workspace will maybe get the singlethreaded feature, when building the entire workspace (but not sure - I still didn't quite get across which crates it's unified).

Just verify that this isn't a problem.

[SteveLauC]

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

---

rt-monoio/Cargo.toml line 17 at r1 (raw file):

Previously, schreter wrote…

BTW, can monoio runtime be part of the same workspace?

The issue I see here is that Cargo unifies features of all dependent crates. Thus, all crates in the workspace will maybe get the singlethreaded feature, when building the entire workspace (but not sure - I still didn't quite get across which crates it's unified).

Just verify that this isn't a problem.

From this issue, cargo will indeed unify the features, as mentioned in the issue, we can use cargo hack as a workaround:

$ cd openraft
$ cargo hack clippy --no-deps --workspace --all-targets     -- -D warnings
info: running `cargo clippy --no-deps --all-targets -- -D warnings` on openraft (1/7)
   Compiling openraft-macros v0.10.0 (openraft/macros)
    Checking serde_json v1.0.120
    Checking openraft v0.10.0 (openraft/openraft)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 10.30s

info: running `cargo clippy --no-deps --all-targets -- -D warnings` on openraft-macros (2/7)
    Checking openraft-macros v0.10.0 (openraft/macros)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.40s

info: running `cargo clippy --no-deps --all-targets -- -D warnings` on tests (3/7)
    Checking openraft v0.10.0 (openraft/openraft)
    Checking tests v0.10.0 (openraft/tests)
    Checking openraft-memstore v0.10.0 (openraft/stores/memstore)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 9.96s

info: running `cargo clippy --no-deps --all-targets -- -D warnings` on openraft-memstore (4/7)
    Checking openraft v0.10.0 (openraft/openraft)
    Checking openraft-memstore v0.10.0 (openraft/stores/memstore)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 5.93s

info: running `cargo clippy --no-deps --all-targets -- -D warnings` on openraft-rocksstore (5/7)
    Checking openraft-rocksstore v0.1.0 (/home/steve/Documents/workspace/INFINI/openraft/stores/rocksstore)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.38s

info: running `cargo clippy --no-deps --all-targets -- -D warnings` on openraft-sledstore (6/7)
    Checking openraft v0.10.0 (openraft/openraft)
    Checking openraft-sledstore v0.10.0 (openraft/stores/sledstore)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 6.49s

info: running `cargo clippy --no-deps --all-targets -- -D warnings` on openraft-rt-monoio (7/7)
   Compiling openraft-macros v0.10.0 (openraft/macros)
    Checking openraft v0.10.0 (openraft/openraft)
    Checking openraft-rt-monoio v0.10.0 (openraft/rt-monoio)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 5.08s

[drmingdrmer]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @schreter and @SteveLauC)

---

Cargo.toml line 61 at r1 (raw file):

]
exclude = [
    "cluster_benchmark",

As @schreter suggested, it would be simpler to put rt-monoio in the exclude section instead of making it a member of the workspace. This way, the singlethreaded feature won't be enabled when compiling the workspace. However, in the CI, rt-monoio must be tested explicitly, just like the examples/*.

Code quote:

    "rt-monoio",
]
exclude = [
    "cluster_benchmark",

[SteveLauC]

Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @drmingdrmer and @schreter)

---

Cargo.toml line 61 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

As @schreter suggested, it would be simpler to put rt-monoio in the exclude section instead of making it a member of the workspace. This way, the singlethreaded feature won't be enabled when compiling the workspace. However, in the CI, rt-monoio must be tested explicitly, just like the examples/*.

Done

---

rt-monoio/src/lib.rs line 109 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Right...

Done.

[drmingdrmer]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @schreter)

[SteveLauC]

I am wondering should we have integration tests for this crate?

[drmingdrmer]

I am wondering should we have integration tests for this crate?

Integration test can be done by replacing this runtime setup with feature-flag enabled runtime, and running the tests/ crate with monoio. tests crate needs a new feature flag to disable tokio and enable monoio:

https://github.com/datafuselabs/openraft/blob/4a33b5f0b6236098121a44e736152eef25ec1cfa/tests/tests/fixtures/mod.rs#L88-L112

[SteveLauC]

Integration test can be done by replacing this runtime setup with feature-flag enabled runtime, and running the tests/ crate with monoio. tests crate needs a new feature flag to disable tokio and enable monoio:

Besides this, Looks like we still need to:

1. Add a monoio-rt feature to the memstore crate
2. Replace the tokio::* stuff used in the integration tests with counterparts of the configured runtime.

I tried it a little bit, step 2 seems hard to do, looks like we have to "hack" those async test functions' signatures to add a generic runtime type so that we can use the stuff configured in it:

#[test_harness::test(harness = ut_harness)]
async fn conflict_with_empty_entries<Rt: AsyncRuntime>(: Rt) -> Result<()> {
   Rt::sleep(Duration::from_secs(1)).await;
    Ok(())
}

And it seems that this is not something we can do with ut_harness(), I guess we have to write our proc macro rather than using the macro provided by the test_harness crate, this macro should turn:

#[our_test_macro]
async fn conflict_with_empty_entries<Rt: AsyncRuntime>(: Rt) -> Result<()> {
   Rt::sleep(Duration::from_secs(1)).await;
    Ok(())
}

into:

#[test]
fn conflict_with_empty_entries() {
    #[allow(clippy::let_unit_value)]
    let _g = init_default_ut_tracing();

    async fn conflict_with_empty_entries<Rt: AsyncRuntime>(: Rt) -> Result<()> {
       Rt::sleep(Duration::from_secs(1)).await;
        Ok(())
    }

    let res;
    cfg_if::cfg_if! {
        if #[cfg(feature = "monoio-rt")] {
            let mut rt = monoio::RuntimeBuilder::<>::new().enable_all().build().unwrap();
            let rt_val = MonoioRuntime;
            res = rt.block_on(async {
                test(rt_val).await;
            });
        } else {
            let rt = tokio::runtime::Runtime::new().unwrap();
            let rt_val = TokioRuntime;
            res = rt.block_on(async {
                test(rt_val).await;
            });
        }
    }
    if let Err(e) = &res {
        tracing::error!("{} error: {:?}", /*func_name*/, e);
    }
}

We can get the function name through the macro, so no need to use that func_name() function.

[drmingdrmer]

Integration test can be done by replacing this runtime setup with feature-flag enabled runtime, and running the tests/ crate with monoio. tests crate needs a new feature flag to disable tokio and enable monoio:

Besides this, Looks like we still need to:

1. Add a monoio-rt feature to the memstore crate
2. Replace the tokio::* stuff used in the integration tests with counterparts of the configured runtime.

I tried it a little bit, step 2 seems hard to do, looks like we have to "hack" those async test functions' signatures to add a generic runtime type so that we can use the stuff configured in it:

#[test_harness::test(harness = ut_harness)]
async fn conflict_with_empty_entries<Rt: AsyncRuntime>(: Rt) -> Result<()> {
   Rt::sleep(Duration::from_secs(1)).await;
    Ok(())
}

And it seems that this is not something we can do with ut_harness(), I guess we have to write our proc macro rather than using the macro provided by the test_harness crate, this macro should turn:

#[our_test_macro]
async fn conflict_with_empty_entries<Rt: AsyncRuntime>(: Rt) -> Result<()> {
   Rt::sleep(Duration::from_secs(1)).await;
    Ok(())
}

into:

#[test]
fn conflict_with_empty_entries() {
    #[allow(clippy::let_unit_value)]
    let _g = init_default_ut_tracing();

    async fn conflict_with_empty_entries<Rt: AsyncRuntime>(: Rt) -> Result<()> {
       Rt::sleep(Duration::from_secs(1)).await;
        Ok(())
    }

    let res;
    cfg_if::cfg_if! {
        if #[cfg(feature = "monoio-rt")] {
            let mut rt = monoio::RuntimeBuilder::<>::new().enable_all().build().unwrap();
            let rt_val = MonoioRuntime;
            res = rt.block_on(async {
                test(rt_val).await;
            });
        } else {
            let rt = tokio::runtime::Runtime::new().unwrap();
            let rt_val = TokioRuntime;
            res = rt.block_on(async {
                test(rt_val).await;
            });
        }
    }
    if let Err(e) = &res {
        tracing::error!("{} error: {:?}", /*func_name*/, e);
    }
}

We can get the function name through the macro, so no need to use that func_name() function.

The integration test can be done in a separate PR since it is relatively independent of this one.

You do not need to modify the function signature: tests uses memstore::TypeConfig as its type config.
Therefore in functions in the tests crate you can just use AsyncRuntime methods such as TypeConfig::sleep().
https://github.com/datafuselabs/openraft/blob/4a33b5f0b6236098121a44e736152eef25ec1cfa/tests/tests/fixtures/mod.rs#L69

I did not try but it looks like memstore should be able to work with monoio 🤔

[SteveLauC]

You do not need to modify the function signature:

You are right, we can directly use openraft_memstore::TypeConfig in the tests function:

<openraft_memstore::TypeConfig as openraft::type_config::TypeConfigExt>::sleep(Duration::from_secs(1)).await;

The integration test can be done in a separate PR since it is relatively independent of this one.

Get it, I will do this in a separate PR:)

[drmingdrmer]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @schreter)

[SteveLauC]

Emm, I didn't squash my commits:<

[drmingdrmer]

Emm, I didn't squash my commits:<

:( squashed and force pushed.