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 higher level utilities for PushManager and Example for Pub/Sub #1193

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

altanozlu
Copy link
Contributor

I have added two new functions for MultiplexedConnection and ConnectionManager,
I believe this methods will make easier to use RESP3 with PubSub.

@altanozlu altanozlu force-pushed the add-resp3-examples branch 2 times, most recently from da6673d to 2806eea Compare May 25, 2024 10:16
Copy link
Contributor

@nihohit nihohit left a comment

Choose a reason for hiding this comment

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

Since we have time to play with the API until the first version with this is released, I'd prefer that instead of these slight improvements, we'd make an effort to consider what is the right API to expose to the user.
Do we really need to expose the replace_sender function to the user, or can we just have these functions (or similar ones)? Do we really need the internal replacable sender, or can we make the push manager optional and have it come with a single, irreplacable sender?
Should we expose the internal channel to the user, or just some stream abstraction?
We have an API that works, but until the next release is released, it isn't set in stone. Let's use the time to verify that it's the best API we can have.

async fn main() -> redis::RedisResult<()> {
// ?resp3=true enables RESP3 for this connection.
// Since RESP3 protocol is used there is no need for creating different connections when using PubSub commands.
let client = redis::Client::open("redis://127.0.0.1/?resp3=true").unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to this PR - is there a specification for redis URL? I couldn't find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@altanozlu
Copy link
Contributor Author

Since we have time to play with the API until the first version with this is released, I'd prefer that instead of these slight improvements, we'd make an effort to consider what is the right API to expose to the user. Do we really need to expose the replace_sender function to the user, or can we just have these functions (or similar ones)? Do we really need the internal replacable sender, or can we make the push manager optional and have it come with a single, irreplacable sender? Should we expose the internal channel to the user, or just some stream abstraction? We have an API that works, but until the next release is released, it isn't set in stone. Let's use the time to verify that it's the best API we can have.

I believe we should hide pushmanager from user, and I'd keep replaceable sender for reconnections etc.
I'm also still fan of having distinct channels per subscription :)

@altanozlu altanozlu marked this pull request as draft June 5, 2024 19:38
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.

None yet

2 participants