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

Add a Monoio async runtime #1010

Closed
wants to merge 17 commits into from
Closed

Conversation

Miaxos
Copy link
Contributor

@Miaxos Miaxos commented Feb 16, 2024

Starting a work around AsyncRuntime to have a MonoioRuntime available & tested.

The main issue is around is_panic and abort right now, as to properly abort a task by using monoio, you need to cancel the IO with a Canceller.

⚠️ This a still a draft, the end idea is to have monoio available and this PR is also used to find out what steps are needed to achieve this. ⚠️

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

Considerations

  • async-entry worker_threads have no impact on monoio with the current implementation of async-entry

References


This change is Reviewable

@Miaxos Miaxos changed the title DRAFT: Add a monoio async runtime Add a Monoio async runtime Feb 16, 2024
@Miaxos Miaxos mentioned this pull request Feb 16, 2024
@Miaxos Miaxos force-pushed the add-monoio-runtime branch 3 times, most recently from 09a4e46 to 8b7e34b Compare February 16, 2024 15:22
Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice that a new runtime is now being added :-).

BTW, we should probably make all required types part of the runtime, so tokio would be optional as well. Currently, oneshot and mpsc channels as well as some other primitives are taken 1:1 from Tokio.

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


openraft/src/async_runtime.rs line 220 at r1 (raw file):

            //
            // We would need to transmit a `[monoio::io::Canceller]` to the future we want to
            // spawn.

Feel free to propose an API where you can pass the Canceller around properly. Can't it be part of a JoinHandle (i.e., wrap monoio::JoinHandle in a new type)?

Code quote:

            // No abort in monoio task, it's a little complicated, as it's using io-uring when
            // available, you register for update from the kernel, but when a task is dropped, you
            // need to "cancel" this registration to the kernel too.
            //
            // We would need to transmit a `[monoio::io::Canceller]` to the future we want to
            // spawn.

@Miaxos
Copy link
Contributor Author

Miaxos commented Feb 16, 2024

BTW, we should probably make all required types part of the runtime, so tokio would be optional as well.

I do completely agree!

Currently, oneshot and mpsc channels as well as some other primitives are taken 1:1 from Tokio.

👍

Feel free to propose an API where you can pass the Canceller around properly. Can't it be part of a JoinHandle (i.e., wrap monoio::JoinHandle in a new type)?

Yeah, well, you need to have access to the Canceller when you want to abort the task, but you'll also need to have access to the Canceller when you do some IO related to io-uring when you are inside the spawned task, so you need it when you are spawning your task, something like this could be possible:

        #[inline]
        fn spawn<T>(fn: F) -> Self::JoinHandle<T::Output>
        where
            T: Future + OptionalSend + 'static,
            T::Output: OptionalSend + 'static,
            F: FnOnce(&AsyncRuntimeContext) -> T,
        {
          ...
        }

I'll try to have some ideas around, and it would be better to also have integration tests running on this new runtime too (linked to drmingdrmer/async-entry#1 )

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime.rs line 220 at r1 (raw file):

Previously, schreter wrote…

Feel free to propose an API where you can pass the Canceller around properly. Can't it be part of a JoinHandle (i.e., wrap monoio::JoinHandle in a new type)?

It appears infeasible to integrate monoio and tokio given that monoio's
JoinHandle lacks an abort() method.

And only specific futures(created with monoio::io::CancelableAsyncReadRent) in
monoio can be canceled, the others can not.

As a potential solution for Openraft, removing AsyncRuntime::abort() might be
a viable option since its primary use is for terminating the Timer. Instead of
relying on abort(), the Timer can be equipped with a shutdown mechanism
utilizing a oneshot::channel() to issue a termination signal, thus gracefully
ceasing its operation.

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, we should probably make all required types part of the runtime, so tokio would be optional as well. Currently, oneshot and mpsc channels as well as some other primitives are taken 1:1 from Tokio.

@schreter
Is there a reason to abstract tokio::sync::* primitives when they can be directly used in asynchronous runtimes other than Tokio?

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

drmingdrmer added a commit to drmingdrmer/openraft that referenced this pull request Feb 17, 2024
Not all of the async-runtime provide an `abort()` primitive, such as
[`monoio`](https://crates.io/crates/monoio).
In this commit `abort()` is removed in order to allow Openraft to be
compatible with `monoio`.

`Tick` is the only mod that relies on `abort()` for shutdown.
In this commit shutting down is replaced with using `tokio::sync::oneshot::channel`.

Refer to:
- databendlabs#1010 (review)
- The above discussion is part of databendlabs#1010
drmingdrmer added a commit to drmingdrmer/openraft that referenced this pull request Feb 17, 2024
Not all of the async-runtime provide an `abort()` primitive, such as
[`monoio`](https://crates.io/crates/monoio).
In this commit `abort()` is removed in order to allow Openraft to be
compatible with `monoio`.

`Tick` is the only mod that relies on `abort()` for shutdown.
In this commit shutting down is replaced with using `tokio::sync::oneshot::channel`.

Refer to:
- databendlabs#1010 (review)
- The above discussion is part of databendlabs#1010
Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to abstract tokio::sync::* primitives when they can be directly used in asynchronous runtimes other than Tokio?

Yes. Tokio primitives do work in other runtimes, but they are not optimally-supported.

For example, Tokio has its own counters for cooperative multitasking (which, obviously, only make sense and are active if running within Tokio), other runtimes have their own algorithm for how not to block the runtime for too long. The same is true for resource monitoring support (e.g., via tracing in Tokio).

Another issue is that Tokio sync primitives allocate memory at unexpected points. This is OK if you are fine with your program going broken on OOM, but we don't have this luxury :-). We need to handle OOMs properly.

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


openraft/src/async_runtime.rs line 220 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

It appears infeasible to integrate monoio and tokio given that monoio's
JoinHandle lacks an abort() method.

And only specific futures(created with monoio::io::CancelableAsyncReadRent) in
monoio can be canceled, the others can not.

As a potential solution for Openraft, removing AsyncRuntime::abort() might be
a viable option since its primary use is for terminating the Timer. Instead of
relying on abort(), the Timer can be equipped with a shutdown mechanism
utilizing a oneshot::channel() to issue a termination signal, thus gracefully
ceasing its operation.

+1

drmingdrmer added a commit that referenced this pull request Feb 18, 2024
Not all of the async-runtime provide an `abort()` primitive, such as
[`monoio`](https://crates.io/crates/monoio).
In this commit `abort()` is removed in order to allow Openraft to be
compatible with `monoio`.

`Tick` is the only mod that relies on `abort()` for shutdown.
In this commit shutting down is replaced with using `tokio::sync::oneshot::channel`.

Refer to:
- #1010 (review)
- The above discussion is part of #1010
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

monoio JoinHandle does not return a Result<T,E> but just T,

Reviewed 4 of 7 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Miaxos and @schreter)


memstore/src/lib.rs line 412 at r1 (raw file):

    #[tracing::instrument(level = "trace", skip(self, entries))]
    async fn append_to_log<I>(&mut self, entries: I) -> Result<(), StorageError<MemNodeId>>
    where I: IntoIterator<Item = Entry<TypeConfig>> + Send {

Does feature flag monoio implies singlethreaded?
If singlethreaded is enabled for Openraft, Openraft does require Send bound for types.

If it is, monoio feature flag should be declared as monoio = ["singlethreaded"].

And memstore does not use Send bound correctly. It should use openraft::OptionalSend here.

Let me fix this issue first.


openraft/src/async_runtime.rs line 220 at r1 (raw file):

Previously, schreter wrote…

+1

abort() is removed, life become easier:

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


memstore/src/lib.rs line 412 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Does feature flag monoio implies singlethreaded?
If singlethreaded is enabled for Openraft, Openraft does require Send bound for types.

If it is, monoio feature flag should be declared as monoio = ["singlethreaded"].

And memstore does not use Send bound correctly. It should use openraft::OptionalSend here.

Let me fix this issue first.

Oh I see that this PR already specified monoio = ["singlethreaded"].

@drmingdrmer
Copy link
Member

Fix OptionalSend usage:

@Miaxos Miaxos force-pushed the add-monoio-runtime branch 3 times, most recently from 414e1b5 to de2aa06 Compare February 19, 2024 17:08
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 16 of 30 files at r2, all commit messages.
Reviewable status: 16 of 30 files reviewed, 4 unresolved discussions (waiting on @Miaxos and @schreter)


openraft/src/async_runtime.rs line 138 at r2 (raw file):

#[cfg(feature = "monoio")]
pub mod monoio {

This file may expand soon if more runtimes are added.
It would be better to promote async_runtime.rs to a mod dir and put every runtime implementation in a separate file.


tests/tests/append_entries/t10_see_higher_vote.rs line 72 at r2 (raw file):

        let n0 = router.get_raft_handle(&0)?;
        spawn(async move {

It can be just replaced with <TypeConfig as RaftTypeConfig\>::AsyncRuntime::spawn.


tests/tests/append_entries/t11_append_conflicts.rs line 234 at r2 (raw file):

        Err(err) => {
            dbg!(err);
            anyhow::bail!("blbl")

@@ -194,6 +196,7 @@ async fn add_learner_with_set_nodes() -> Result<()> {
/// Because adding learner is also a change-membership operation, a new membership config log will
/// let raft consider the previous membership config log as committed, which is actually not.
#[async_entry::test(worker_threads = 8, init = "init_default_ut_tracing()", tracing_span = "debug")]
#[cfg_attr(feature = "monoio", ignore)] // Crashing the future is causing a whole crash with monoio
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fix

@@ -211,13 +214,13 @@ async fn add_learner_when_previous_membership_not_committed() -> Result<()> {
router.set_network_error(1, true);

let node = router.get_raft_handle(&0)?;
tokio::spawn(async move {
<TypeConfig as RaftTypeConfig>::AsyncRuntime::spawn(async move {
Copy link
Contributor Author

@Miaxos Miaxos Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using it seems to not be a good idea for those tests, at the end we have spawn and spawn_local, when inside a test we could have openraft running with singlethread so each spawn has to be spawn_local but the test is running with multi-threading, so for tests we need spawn. Need to revisit this, I'll revert back to have some spawn abstracted over the feature flag. (wdyt @drmingdrmer @schreter ?)

At the end, I'm wondering, when we should & when we shouldn't use AsyncRuntime implementation in those tests. (like spawn here)

Copy link
Contributor Author

@Miaxos Miaxos Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, my bad, it's not happening when testing with tokio right now due to the fact we never test singlethread with tokio, it was happening on my local test due to a misconfiguration on my end. (I enabled the monoio default feature flag, so it was singlethread even when testing against tokio 😇 )

Still, I would love to hear what you think about this.

Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 30 files at r2, 19 of 26 files at r3, 1 of 2 files at r4, 8 of 8 files at r5, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @drmingdrmer and @Miaxos)


README.md line 4 at r5 (raw file):

    <h1>Openraft</h1>
    <h4>
        Advanced <a href="https://raft.github.io/">Raft</a> in 🦀 Rust using <a href="https://tokio.rs/">Tokio</a> or <a href="https://github.com/bytedance/monoio/">Monoio</a>. Please ⭐ on <a href="https://github.com/datafuselabs/openraft">github</a>!

Split long lines?


openraft/src/lib.rs line 81 at r5 (raw file):

#[cfg(feature = "monoio")] pub use crate::async_runtime::monoio::MonoioRuntime;
pub use crate::async_runtime::tokio::TokioInstant;
pub use crate::async_runtime::tokio::TokioRuntime;

?

BTW do we need MonoioInstant/TokioInstant at all?

Suggestion:

#[cfg(feature = "monoio")] pub use crate::async_runtime::monoio::MonoioInstant;
#[cfg(feature = "monoio")] pub use crate::async_runtime::monoio::MonoioRuntime;
#[cfg(not(feature = "monoio"))] pub use crate::async_runtime::tokio::TokioInstant;
#[cfg(not(feature = "monoio"))] pub use crate::async_runtime::tokio::TokioRuntime;

openraft/src/async_runtime/mod.rs line 10 at r5 (raw file):

use crate::OptionalSync;

pub mod tokio;

Same here - only include if relevant?

Suggestion:

#[cfg(not(feature = "monoio"))] pub mod tokio;

tests/tests/fixtures/runtime.rs line 6 at r5 (raw file):

#[cfg(feature = "monoio")] pub use monoio::spawn;
#[cfg(not(feature = "monoio"))] pub use tokio::spawn;
#[cfg(not(feature = "monoio"))] pub use tokio::sync::oneshot;

Shouldn't these be simply part of the AsyncRuntime and used from there?

Code quote:

#[cfg(feature = "monoio")] pub use local_sync::oneshot;
#[cfg(feature = "monoio")] pub use monoio::spawn;
#[cfg(not(feature = "monoio"))] pub use tokio::spawn;
#[cfg(not(feature = "monoio"))] pub use tokio::sync::oneshot;

tests/tests/membership/t11_add_learner.rs line 217 at r3 (raw file):

Previously, Miaxos (Anthony Griffon) wrote…

Oh, my bad, it's not happening when testing with tokio right now due to the fact we never test singlethread with tokio, it was happening on my local test due to a misconfiguration on my end. (I enabled the monoio default feature flag, so it was singlethread even when testing against tokio 😇 )

Still, I would love to hear what you think about this.

I think we don't need to differentiate between spawn/spawn_local. The AsyncRuntime exposes the "right" spawn based on the type of the runtime. I.e., always use spawn from RaftTypeConfig::AsyncRuntime.

The singlethreaded feature was originally built for our proprietary runtime, which is kind of similar to monoio in the sense that we have thread-per-core and use !Send futures.

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r8, 5 of 9 files at r9, 3 of 3 files at r11, all commit messages.
Reviewable status: 32 of 36 files reviewed, 8 unresolved discussions (waiting on @Miaxos and @schreter)


openraft/src/lib.rs line 81 at r5 (raw file):

Previously, schreter wrote…

?

BTW do we need MonoioInstant/TokioInstant at all?

I do not get it either why to import MonoioInstant/MonoioRuntime. 🤔


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

use crate::OptionalSync;

#[cfg(not(feature = "monoio"))] pub mod tokio;

Shall we add a feature tokio and make it enabled by default?
There will be more AsyncRuntimeprovided in future. Then negative feature assertion like#[cfg(not(feature = "monoio"))]` does not support more than two runtimes.

BTW what about name runtime related feature flags in form of rt-monoio and rt-tokio?
So that developers can find out all runtime related flags quickly.


openraft/src/core/raft_core.rs line 932 at r11 (raw file):

            // an issue
            #[cfg(feature = "monoio")]
            monoio::time::sleep(Duration::from_millis(0)).await;

It is the issue with using tokio channel inside monoio runtime, right?


tests/tests/append_entries/t11_append_conflicts.rs line 229 at r9 (raw file):

    C: RaftTypeConfig,
    LS: RaftLogStorage<C>,
    C::NodeId: Sync + Send,

Why does it need Sync+Send bound? 🤔

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 26 files at r3.
Reviewable status: 32 of 36 files reviewed, 9 unresolved discussions (waiting on @Miaxos and @schreter)


tests/tests/life_cycle/t11_shutdown.rs line 41 at r11 (raw file):

/// A panicked RaftCore should also return a proper error the next time accessing the `Raft`.
#[async_entry::test(worker_threads = 8, init = "init_default_ut_tracing()", tracing_span = "debug")]
#[cfg_attr(feature = "monoio", ignore)]

Suggestion:

// monoio `JoinHandle` does not return an `Err` when there is a panic.
#[cfg_attr(feature = "monoio", ignore)]

Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 11 of 12 files at r6, 3 of 3 files at r7, 1 of 2 files at r8, 6 of 9 files at r9, 1 of 2 files at r10, 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @drmingdrmer and @Miaxos)


openraft/src/lib.rs line 81 at r5 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

I do not get it either why to import MonoioInstant/MonoioRuntime. 🤔

Then I'd suggest removing the two imports and check where it fails to compile. Those locations then need to use async runtime from the config.


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Shall we add a feature tokio and make it enabled by default?
There will be more AsyncRuntimeprovided in future. Then negative feature assertion like#[cfg(not(feature = "monoio"))]` does not support more than two runtimes.

BTW what about name runtime related feature flags in form of rt-monoio and rt-tokio?
So that developers can find out all runtime related flags quickly.

Yes, good ideas.


openraft/src/async_runtime/monoio.rs line 92 at r11 (raw file):

impl<T: OptionalSend> Debug for MonoioOneshotSender<T> {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_tuple("MonoioSendWrapper").finish()

Suggestion:

        f.debug_tuple("MonoioOneshotSender").finish()

openraft/src/async_runtime/tokio.rs line 100 at r11 (raw file):

impl<T: OptionalSend> Debug for TokioOneShotSender<T> {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_tuple("TokioSendWrapper").finish()

BTW, shouldn't it be TokioOneshotSender to harmonize with other names (i.e., lowercase shot)?

Suggestion:

        f.debug_tuple("TokioOneShotSender").finish()

Copy link
Collaborator

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

Previously, schreter wrote…

Yes, good ideas.

This looks like a good idea, do you still want this?


openraft/src/core/raft_core.rs line 932 at r11 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

It is the issue with using tokio channel inside monoio runtime, right?

Also curious about this🤔

@SteveLauC
Copy link
Collaborator

Hi @Miaxos, would you like to finish this, or do you mind me picking it up? I need this at work so I plan to do this in the following days if I am allowed to pick this up:)

Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

Previously, SteveLauC (SteveLauC) wrote…

This looks like a good idea, do you still want this?

It still makes sense to have positive config checks instead of negative. If there will be third type of runtime, then all these special handlings will get more complex.

OTOH, positive flags have the danger of bringing both into scope and then the compilation will fail on the check of double runtime.

But, in general, we should not have too many special handlings :-). In our project, we use a completely different runtime (closer to monoio, though, but with tokio-compatible APIs and primitives). This is passed via AsyncRuntime implementation to openraft and openraft formally uses tokio runtime.

Thus, the special handling we have should be probably really limited to these two lines to import the default runtime. Any other #[cfg(...)] on runtime should be replaced by associated methods on AsyncRuntime doing the needful for specialization for the runtime.


openraft/src/core/raft_core.rs line 932 at r11 (raw file):

Previously, SteveLauC (SteveLauC) wrote…

Also curious about this🤔

Possibly, there is a watch which is never marked as received and thus always hits causing it to go to a hot loop? Or a bug in monoio? No idea, I didn't look into details.

Regarding the above, this would be something like AsyncRuntime::optional_yield_in_runtime_loop().await; which would be empty for tokio (and default-implemented empty on AsyncRuntime) and filled for monoio, with the documentation why do we need this hack once it's established we need it and why.

Copy link
Collaborator

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

OTOH, positive flags have the danger of bringing both into scope and then the compilation will fail on the check of double runtime.

Yeah, this is something we need to consider. I think it should be allowed to enable 2 or multiple runtime features, and it would be safe if enabling a feature only introduces the needed optional dependencies and exposes the corresponding Openraft runtime type.

In this PR's implementation, I saw that macro declare_raft_types! uses different default runtime with different feature, this is something I think we need to avoid.

In our project, we use a completely different runtime (closer to monoio, though, but with tokio-compatible APIs and primitives).

Sounds like yet another io_uring based runtime, I guess it is not open source?

Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):
It would be maybe better to move runtime impls to separate crates and depending on features import one of the separate crates as a default. Then, make tokio-rt as a default feature. Then, for instance in our project, we can use default-features = false to include no runtime, since we anyway specify our own.

Or, get rid of features altogether, pack runtimes to separate crates and force the user to specify the runtime and link appropriate runtime crate (that would be the cleanest approach).

Sounds like yet another io_uring based runtime, I guess it is not open source?

Unfortunately, it's not open source. It is using pluggable I/O driver, io_uring for Linux, mio for MacOS and where applicable, we plan to use DPDK/SPDK-based user-space networking and disk access.

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

Previously, schreter wrote…

It would be maybe better to move runtime impls to separate crates and depending on features import one of the separate crates as a default. Then, make tokio-rt as a default feature. Then, for instance in our project, we can use default-features = false to include no runtime, since we anyway specify our own.

Or, get rid of features altogether, pack runtimes to separate crates and force the user to specify the runtime and link appropriate runtime crate (that would be the cleanest approach).

Sounds like yet another io_uring based runtime, I guess it is not open source?

Unfortunately, it's not open source. It is using pluggable I/O driver, io_uring for Linux, mio for MacOS and where applicable, we plan to use DPDK/SPDK-based user-space networking and disk access.

Without a new feature-flag would be better to me.:)

Copy link
Collaborator

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

It would be maybe better to move runtime impls to separate crates and depending on features import one of the separate crates as a default. Then, make tokio-rt as a default feature. Then, for instance in our project, we can use default-features = false to include no runtime, since we anyway specify our own.

Or, get rid of features altogether, pack runtimes to separate crates and force the user to specify the runtime and link appropriate runtime crate (that would be the cleanest approach).

Kinda think splitting runtimes into separate crates would be an overkill, we will still gate them with features, and we have to do hacks to stop user from import them without through Openraft.

Make tokio-rt the default runtime feature, and enforce that only one runtime feature will be enabled (we can through a compile-time error via the compile_error!() macro) looks feasible 🤔

Copy link
Collaborator

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

Without a new feature-flag would be better to me.:)

Would you like to elaborate on this a bit? By this new feature-flag, do you mean this monoio feature, or the tokio feature we have been talking in this thread?

Copy link
Collaborator

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

Make tokio-rt the default runtime feature, and enforce that only one runtime feature will be enabled (we can through a compile-time error via the compile_error!() macro) looks feasible 🤔

s/through/throw/g

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

Previously, SteveLauC (SteveLauC) wrote…

Make tokio-rt the default runtime feature, and enforce that only one runtime feature will be enabled (we can through a compile-time error via the compile_error!() macro) looks feasible 🤔

s/through/throw/g

At least the monoio feature-flag is not required to me. If a runtime can be configured with RaftTypeConfig. Then there is no need to introduce a feature-flag.

The by-default-use-tokio feature flag might be useful: the Openraft user won't want to spend time compiling tokio if they were using another async-runtime.

Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

At least the monoio feature-flag is not required to me. If a runtime can be configured with RaftTypeConfig. Then there is no need to introduce a feature-flag.

The by-default-use-tokio feature flag might be useful: the Openraft user won't want to spend time compiling tokio if they were using another async-runtime.

Ok, so what about having a single flag set by default (tokio-rt) which would compile tokio runtime into openraft? Then, turning off the default features would require the user to specify the runtime (i.e., no default runtime in types) and do not import tokio as a dependency.

The monoio runtime would be in a separate crate and the test with monoio would import this crate and use default-features = false to compile the test.

Copy link
Collaborator

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):
Making tokio the default runtime makes sense.

At least the monoio feature-flag is not required to me. If a runtime can be configured with RaftTypeConfig. Then there is no need to introduce a feature-flag.

The monoio runtime would be in a separate crate and the test with monoio would import this crate and use default-features = false to compile the test.

If we don't introduce a feature for monoio, then to avoid pulling it in, looks like we indeed need to put it in a separate crate🤔

Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

If we don't introduce a feature for monoio, then to avoid pulling it in, looks like we indeed need to put it in a separate crate🤔

Correct. And, where is the problem?
BTW, interestingly, the more crates, the faster the compilation in general :-).

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

Ok, so what about having a single flag set by default (tokio-rt) which would compile tokio runtime into openraft? Then, turning off the default features would require the user to specify the runtime (i.e., no default runtime in types) and do not import tokio as a dependency.

The monoio runtime would be in a separate crate and the test with monoio would import this crate and use default-features = false to compile the test.

Make sense :)

Copy link
Collaborator

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


openraft/src/async_runtime/mod.rs line 10 at r11 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Ok, so what about having a single flag set by default (tokio-rt) which would compile tokio runtime into openraft? Then, turning off the default features would require the user to specify the runtime (i.e., no default runtime in types) and do not import tokio as a dependency.

The monoio runtime would be in a separate crate and the test with monoio would import this crate and use default-features = false to compile the test.

Make sense :)

Get it, I will follow this design when I pick it up. 😆

@SteveLauC
Copy link
Collaborator

Hi @drmingdrmer and @schreter , @Miaxos hasn't been active for a while so I don't think we can have him here, and based on our preliminary work (discussion and PRs), a lot of things have changed, do you mind me picking this PR up?

@drmingdrmer
Copy link
Member

Hi @drmingdrmer and @schreter , @Miaxos hasn't been active for a while so I don't think we can have him here, and based on our preliminary work (discussion and PRs), a lot of things have changed, do you mind me picking this PR up?

Thank you. Please do so.
It looks like @Miaxos has been busy on something else for a while :)

Copy link
Collaborator

@xp-trumpet xp-trumpet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this PR can be closed

@SteveLauC
Copy link
Collaborator

It looks like this PR can be closed

Yeah, this can be closed now:)

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 this pull request may close these issues.

5 participants