-
Notifications
You must be signed in to change notification settings - Fork 160
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
Conversation
ba455eb
to
37a921e
Compare
37a921e
to
09a4e46
Compare
09a4e46
to
8b7e34b
Compare
There was a problem hiding this 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.
I do completely agree!
👍
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 ) |
There was a problem hiding this 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 aJoinHandle
(i.e., wrapmonoio::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.
There was a problem hiding this 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)
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
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
There was a problem hiding this 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 anabort()
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 theTimer
. Instead of
relying onabort()
, theTimer
can be equipped with a shutdown mechanism
utilizing aoneshot::channel()
to issue a termination signal, thus gracefully
ceasing its operation.
+1
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
There was a problem hiding this 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:
There was a problem hiding this 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
impliessinglethreaded
?
Ifsinglethreaded
is enabled for Openraft, Openraft does requireSend
bound for types.If it is,
monoio
feature flag should be declared asmonoio = ["singlethreaded"]
.And
memstore
does not useSend
bound correctly. It should useopenraft::OptionalSend
here.Let me fix this issue first.
Oh I see that this PR already specified monoio = ["singlethreaded"]
.
414e1b5
to
de2aa06
Compare
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
d254005
to
cf7f71e
Compare
There was a problem hiding this 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 themonoio
default feature flag, so it wassinglethread
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.
ab51c74
to
f9cad89
Compare
There was a problem hiding this 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? 🤔
There was a problem hiding this 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)]
There was a problem hiding this 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
andrt-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()
There was a problem hiding this 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🤔
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:) |
There was a problem hiding this 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.
There was a problem hiding this 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 withtokio
-compatible APIs and primitives).
Sounds like yet another io_uring based runtime, I guess it is not open source?
There was a problem hiding this 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.
There was a problem hiding this 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 usedefault-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.:)
There was a problem hiding this 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 usedefault-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 🤔
There was a problem hiding this 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?
There was a problem hiding this 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 thecompile_error!()
macro) looks feasible 🤔
s/through/throw/g
There was a problem hiding this 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 thecompile_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.
There was a problem hiding this 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.
There was a problem hiding this 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 withmonoio
would import this crate and usedefault-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🤔
There was a problem hiding this 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 :-).
There was a problem hiding this 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 compiletokio
runtime intoopenraft
? Then, turning off the default features would require the user to specify the runtime (i.e., no default runtime in types) and do not importtokio
as a dependency.The
monoio
runtime would be in a separate crate and the test withmonoio
would import this crate and usedefault-features = false
to compile the test.
Make sense :)
There was a problem hiding this 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 compiletokio
runtime intoopenraft
? Then, turning off the default features would require the user to specify the runtime (i.e., no default runtime in types) and do not importtokio
as a dependency.The
monoio
runtime would be in a separate crate and the test withmonoio
would import this crate and usedefault-features = false
to compile the test.Make sense :)
Get it, I will follow this design when I pick it up. 😆
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. |
There was a problem hiding this 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
Yeah, this can be closed now:) |
Starting a work around
AsyncRuntime
to have aMonoioRuntime
available & tested.The main issue is around
is_panic
andabort
right now, as to properlyabort
a task by usingmonoio
, you need to cancel the IO with aCanceller
.monoio
available and this PR is also used to find out what steps are needed to achieve this.Checklist
Considerations
worker_threads
have no impact onmonoio
with the current implementation ofasync-entry
References
This change is