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
Async-ify the API #232
Async-ify the API #232
Conversation
Outcome of rust-lang/rust#63778 might affect what kind of API we want for the query builders as they are quite inconvenient atm |
5c7c8b4
to
c5134d8
Compare
I changed the async |
c47ee2a
to
f460ff8
Compare
906281b
to
12811c3
Compare
Are there any plans around merging/releasing an alpha with this? |
This is probably as good as I can make this PR for now so 👍 on releasing an alpha with this. Could use a review however. |
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.
Here's a drive-by review. Do with it whatever you like. 👍 It ended up mostly being about style issues. The core async/await stuff looks broadly right to me.
Right now the README still states:
That would probably need to be changed, right? |
FWIW, I've just finished my work on updating the bb8 connection pool to async/await, which includes support for this branch in the bb8-redis crate. |
I'm currently trying out your branch and will post feedback here as I gain more experience with the library. The first thing that I noticed: The difference between (If I understand it correctly, with |
Thanks for the feedback @dbrgn . I renamed it to multiplexed (like https://stackexchange.github.io/StackExchange.Redis/PipelinesMultiplexers.html) instead of shared which should explain it better, and added some more documentation. |
Thanks @Marwes! The new name makes it clearer 🙂 |
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.
How does the multiplexed connection deal with blocking operations like BLPOP, BRPOP and BRPOPLPUSH? In the docs for the SE.Redis library it's mentioned that those are explicitly not supported when multiplexing. However, in this case since the redis commands are strings we cannot prevent the user from calling those commands, right?
src/client.rs
Outdated
/// A multiplexed connection can be cloned, allowing requests to be be sent concurrently | ||
/// on the same underlying connection (tcp/unix socket). | ||
/// | ||
/// Requires an executor to spawn a task that drives the underlying connection. |
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.
I wonder if a note should be added that get_multiplexed_async_connection
is also available, but only if the executor
feature is enabled. Might be a bit hard to discover otherwise.
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.
Changed it around a bit to be tokio-executor
instead and mentioned the feature.
We could prevent those and return |
Yep, since the API is rather lower-level we can probably ignore this. However, documenting the limitation in the |
Hey all! Rust 1.39 is out and |
Waiting for an alpha of tokio which uses futures-0.3 seems prudent (tokio-rs/tokio#1708) otherwise this could be released as an alpha whenever (in my opinion, I have nothing further to change). |
b5e499a
to
35b5a28
Compare
Explicitly specify that it uses tokio instead and let the executor agnostic version take the simpler, async name. BREAKING CHANGE Change get_multiplexed_async_connection to get_multiplexed_tokio_connection
75bfe24
to
7c5bfe9
Compare
Awesome, thanks to everyone involved for landing this feature! 🎉 |
Regressed in redis-rs#232 (there is a comment explaining what should be done, only it wasn't anymore) Fixes redis-rs#275
Regressed in redis-rs#232 (there is a comment explaining what should be done, only it wasn't anymore) Fixes redis-rs#275
Regressed in redis-rs#232 (there is a comment explaining what should be done, only it wasn't anymore) Fixes redis-rs#275
With async/await we can take references in the async api and avoid clones and
moves in many cases.
Based on #231