-
Notifications
You must be signed in to change notification settings - Fork 593
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 async pubsub manager. #1393
base: main
Are you sure you want to change the base?
Conversation
dd4444d
to
7c9deea
Compare
026d6fa
to
7ede7f9
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.
I just looked through, I will have detailed view & testing this weekend. Looks clean and documented :)
|
||
/// The configuration for reconnect mechanism and request timing for the [PubSubManager] | ||
#[derive(Clone, Debug)] | ||
pub struct PubsubManagerConfig { |
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.
not directly related to this PR but with PubsubManagerConfig change, there will be 3 different retry configuration in the crate, maybe later on they can merged or share things ?
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.
definitely.
redis/src/aio/pubsub_manager.rs
Outdated
let packed_cmd = cmd.get_packed_command(); | ||
async move { sink.send_recv(packed_cmd).await } | ||
}); | ||
// TODO - should we handle errors better than just failing? how? |
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.
Currently this PubSubManager only accepts PSUBSCRIBE,SUBSCRIBE and if those channels were able to successfully subscribe before there should be no problem reconnecting again, except some network issues etc. which retrying again feels safer imho.
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.
you're right. clarified the docs.
7ede7f9
to
4f7a756
Compare
A connection manager for pubsub.
4f7a756
to
9ca9a39
Compare
I'm having a hard time stabilizing this, so this probably won't make it into 0.28 |
A connection manager for pubsub.
Based over #1392 & #1341