Refactor: gate tokio rt with feature tokio-rt

[SteveLauC]

What does this PR do

Gates the tokio runtime with a new feature tokio-rt, and makes it a default feature.

1. Replaces tokio_select!() with futures::select_biased!()
2. Everything related to Tokio is now hidden behind the tokio-rt feature.

About the declare_raft_types! macro

I haven't done anything to this because I haven't found a good way to update it, currently, with the default (tokio-rt) feature off, using this macro without providing a runtime would result in a compile error:

error[E0412]: cannot find type `TokioRuntime` in module `$crate::impls`
 --> src/main.rs:3:1
  |
3 | declare_raft_types!(pub TypeConfig);
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not found in `$crate::impls`
  |
note: found an item that was configured out
 --> /home/steve/Documents/workspace/INFINI/openraft/openraft/src/impls/mod.rs:8:51
  |
8 | pub use crate::type_config::async_runtime::impls::TokioRuntime;
  |                                                   ^^^^^^^^^^^^
  = note: the item is gated behind the `tokio-rt` feature
  = note: this error originates in the macro `$crate::declare_raft_types` which comes from the expansion of the macro `declare_raft_types` (in Nightly builds, run with -Z macro-backtrace for more info)

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]

Reviewable status: 0 of 15 files reviewed, 1 unresolved discussion

---

openraft/src/raft/mod.rs line 171 at r1 (raw file):

                (Node         , , $crate::impls::BasicNode              ),
                (Entry        , , $crate::impls::Entry<Self>            ),
                (SnapshotData , , std::io::Cursor<Vec<u8>>                       ),

Use the full path here so that users won't need to manually import it when using this default value.

[schreter]

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

---

openraft/src/metrics/wait.rs line 69 at r1 (raw file):

            futures::select_biased! {
                _ = delay.fuse() => {
                tracing::debug!( "id={} timeout wait {:} latest: {}", latest.id, msg.to_string(), latest );

You might want to correct indentation here.

---

openraft/src/network/snapshot_transport.rs line 5 at r1 (raw file):

use std::future::Future;
#[cfg(feature = "tokio-rt")]

Instead of many markers, you could group all relevant imports in a private mod tokio_deps or so and then use tokio_imports::*, so you'll only need two cfg markers.

Similarly, the tokio_deps module could also contain relevant impls from below, since they can be anywhere in the crate.

---

openraft/src/raft/mod.rs line 33 at r1 (raw file):

use std::time::Duration;

use async_lock::Mutex;

I'd suggest to move this to the AsyncRuntime, alongside other sync primitives we already have there.

[SteveLauC]

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

---

openraft/src/metrics/wait.rs line 69 at r1 (raw file):

Previously, schreter wrote…

You might want to correct indentation here.

Thanks for catching it! Done:)

---

openraft/src/network/snapshot_transport.rs line 5 at r1 (raw file):

Previously, schreter wrote…

Instead of many markers, you could group all relevant imports in a private mod tokio_deps or so and then use tokio_imports::*, so you'll only need two cfg markers.

Similarly, the tokio_deps module could also contain relevant impls from below, since they can be anywhere in the crate.

It seems that if we move the implementation to this private module, then we don't need to use tokio_imports::* as the things that need tokio are already in the module.

Done.

---

openraft/src/raft/mod.rs line 33 at r1 (raw file):
Emm, previously @drmingdrmer said:

It's alright to mock a Mutex instead of requiring user to implement it.

Perhaps we could discuss a bit before adding it:)

[drmingdrmer]

Reviewed 10 of 15 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: 14 of 15 files reviewed, 5 unresolved discussions (waiting on @schreter and @SteveLauC)

---

openraft/src/engine/log_id_list.rs line 305 at r2 (raw file):

    pub(crate) fn key_log_ids(&self) -> &[LogId<C::NodeId>] {
        &self.key_log_ids
    }

Why does this method requires tokio-rs?

Code quote:

    #[cfg(feature = "tokio-rt")]
    pub(crate) fn key_log_ids(&self) -> &[LogId<C::NodeId>] {
        &self.key_log_ids
    }

---

openraft/src/raft/mod.rs line 33 at r1 (raw file):

Previously, SteveLauC (SteveLauC) wrote…

Emm, previously @drmingdrmer said:

It's alright to mock a Mutex instead of requiring user to implement it.

Perhaps we could discuss a bit before adding it:)

Let's discuss here:

• Refactor: Implement Custom Async Mutex Using Oneshot Channel #1208

---

openraft/src/raft/mod.rs line 171 at r1 (raw file):

Previously, SteveLauC (SteveLauC) wrote…

Use the full path here so that users won't need to manually import it when using this default value.

Great!

---

openraft/src/type_config/async_runtime/mod.rs line 12 at r2 (raw file):

    #[cfg(feature = "tokio-rt")]
    pub use tokio_runtime::TokioRuntime;
}

The impls module is specifically designed for use with Tokio. It would be more appropriately named tokio_impls. Additionally, the relevant attribute should be applied to this module to clarify its purpose.

Code quote:

pub(crate) mod impls {
    #[cfg(feature = "tokio-rt")]
    mod tokio_runtime;

    #[cfg(feature = "tokio-rt")]
    pub use tokio_runtime::TokioRuntime;
}

[SteveLauC]

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

---

openraft/src/engine/log_id_list.rs line 305 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Why does this method requires tokio-rs?

Sorry for being unclear, the attribute should be:

    #[cfg_attr(not(feature = "tokio-rt"), allow(dead_code))]

as this method will only be used under feature tokio-rt.

---

openraft/src/type_config/async_runtime/mod.rs line 12 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

The impls module is specifically designed for use with Tokio. It would be more appropriately named tokio_impls. Additionally, the relevant attribute should be applied to this module to clarify its purpose.

Done.

[drmingdrmer]

Reviewed 1 of 2 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @schreter)

[schreter]

Reviewed 2 of 2 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @drmingdrmer and @SteveLauC)

---

openraft/src/engine/log_id_list.rs line 305 at r2 (raw file):

Previously, SteveLauC (SteveLauC) wrote…

Sorry for being unclear, the attribute should be:

    #[cfg_attr(not(feature = "tokio-rt"), allow(dead_code))]

as this method will only be used under feature tokio-rt.

I think it would make sense to make this method simply pub, since it's used in many tests. Tests with other runtimes might want to request the log IDs as well.

---

openraft/src/network/snapshot_transport.rs line 5 at r1 (raw file):

Previously, SteveLauC (SteveLauC) wrote…

It seems that if we move the implementation to this private module, then we don't need to use tokio_imports::* as the things that need tokio are already in the module.

Done.

I didn't check the large move, but I assume it's just a move. If not, please mark code needing extra review.

[SteveLauC]

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

---

openraft/src/engine/log_id_list.rs line 305 at r2 (raw file):

Previously, schreter wrote…

I think it would make sense to make this method simply pub, since it's used in many tests. Tests with other runtimes might want to request the log IDs as well.

I think we can do this in the future when the support for other runtimes is added as we are currently not sure how to test a new async runtime

---

openraft/src/network/snapshot_transport.rs line 5 at r1 (raw file):

Previously, schreter wrote…

I didn't check the large move, but I assume it's just a move. If not, please mark code needing extra review.

It is just a move, please see this commit.

[schreter]

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

---

openraft/src/engine/log_id_list.rs line 305 at r2 (raw file):

Previously, SteveLauC (SteveLauC) wrote…

I think we can do this in the future when the support for other runtimes is added as we are currently not sure how to test a new async runtime

Of course, you can keep it as-is. Just most likely it will be needed sooner or later. But, not sure.

[drmingdrmer]

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

---

openraft/src/raft/mod.rs line 46 at r4 (raw file):

#[cfg(feature = "tokio-rt")]
use crate::async_runtime::mutex::Mutex;

Mutex is defined by async-runtime thus it does no need tokio-rs feature enabled. no?

Code quote:

#[cfg(feature = "tokio-rt")]
use crate::async_runtime::mutex::Mutex;

[SteveLauC]

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

---

openraft/src/engine/log_id_list.rs line 305 at r2 (raw file):

Previously, schreter wrote…

Of course, you can keep it as-is. Just most likely it will be needed sooner or later. But, not sure.

Done.

---

openraft/src/raft/mod.rs line 46 at r4 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Mutex is defined by async-runtime thus it does no need tokio-rs feature enabled. no?

It is currently only used under the feature tokio-rt, do I need to change it to:

#[cfg_attr(not(feature = "tokio-rt"), allow(unused_imports))]

to make it more clear?

[drmingdrmer]

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

---

openraft/src/raft/mod.rs line 46 at r4 (raw file):

Previously, SteveLauC (SteveLauC) wrote…

It is currently only used under the feature tokio-rt, do I need to change it to:

#[cfg_attr(not(feature = "tokio-rt"), allow(unused_imports))]

to make it more clear?

If it is not required by this file but only by a function,
you can just move this use to the function install_snapshot.

[SteveLauC]

Reviewable status: 15 of 16 files reviewed, 2 unresolved discussions (waiting on @drmingdrmer)

---

openraft/src/raft/mod.rs line 46 at r4 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

If it is not required by this file but only by a function,
you can just move this use to the function install_snapshot.

That's a style I really love, extremely helpful when you are in a big bowl of cfg soup, done!

[drmingdrmer]

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

[SteveLauC]

Let me squash the commits:)

Hi @schreter, do you want to take another look at this PR before I do that?

[schreter]

Reviewed 8 of 9 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SteveLauC)

[SteveLauC]

Commits squashed

[SteveLauC]

Well, I cannot reproduce that test failure locally:>

[drmingdrmer]

Well, I cannot reproduce that test failure locally:>

These tests are run with OPENRAFT_NETWORK_SEND_DELAY set to 30 milliseconds to simulate network delay.
Did you run it locally with this environment variable set?
OPENRAFT_NETWORK_SEND_DELAY=30 cargo test

I'll have a look on the logs to see what's going wrong:
The log can be found in the artifacts archive: https://github.com/datafuselabs/openraft/actions/runs/10207624483?pr=1207#artifacts

[SteveLauC]

Thanks for showing me that parameter!

I just tried 17 times with that option, but I didn't reproduce the failure that occurred in CI, instead, I got this one:

failures:
    t11_add_learner::check_learner_after_leader_transferred

test result: FAILED. 39 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 13.68sj

Though this test's log was mixed with previous tests' log so I cannot provide it 😞

[drmingdrmer]

Thanks for showing me that parameter!

I just tried 17 times with that option, but I didn't reproduce the failure that occurred in CI, instead, I got this one:

failures:
    t11_add_learner::check_learner_after_leader_transferred

test result: FAILED. 39 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 13.68sj

Though this test's log was mixed with previous tests' log so I cannot provide it 😞

It appears that these failures are not related to this PR. I will investigate to determine the cause.

[drmingdrmer]

The failure should be fixed in:

• Fix: (0.10) Clarify that receiving an equal vote does not grant leadership. #1220

[drmingdrmer]

The failure should be fixed in:

• Fix: (0.10) Clarify that receiving an equal vote does not grant leadership. #1220

Merged and there should not be such issue any more 🤔

[SteveLauC]

Rebased.

[drmingdrmer]

Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SteveLauC)