databendlabs / openraft

rust raft with improvements
Apache License 2.0
1.41k stars 158 forks source link

Feature: monoio runtime support #1216

Closed SteveLauC closed 3 months ago

SteveLauC commented 4 months ago

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


This change is Reviewable

SteveLauC commented 4 months ago

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 commented 4 months ago

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.

SteveLauC commented 3 months ago

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

drmingdrmer commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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:)

SteveLauC commented 3 months ago

Emm, I didn't squash my commits:<

drmingdrmer commented 3 months ago

Emm, I didn't squash my commits:<

:( squashed and force pushed.