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 redis sentinel connection pool #339

Merged
merged 8 commits into from
Aug 29, 2024
Merged

Conversation

smf8
Copy link
Contributor

@smf8 smf8 commented Jul 16, 2024

Hello again, I've come up with this draft to add a sentinel connection pool according to #338.

It's currently in a draft state, meaning it might have some issues but I wanted to share the general idea with you. It'd be great if you could review this and point out whether I missed anything or if anything could be improved.

I apologize again if I made any rookie mistakes but I'm fairly new to Rust and I'd like to contribute as well as I can.

@bikeshedder
Copy link
Owner

LGTM except for the linter and doc errors.

btw. you also forgot to change the comment in the source code from cluster to sentinel.

It would be nice if there was a way to share the configuration from redis, redis::cluster and redis::sentinel somehow. One way would be to add support for multiple URLs for the regular redis client as well. It could just try the provided connection infos in order and fail if neither one worked. That way one config could be used for all three implementations. 🤔

@smf8
Copy link
Contributor Author

smf8 commented Jul 24, 2024

Hi, Thank you for the review. Sorry, I've postponed some lint/doc issues for later and wanted to see if the general idea looked good.

I'll try to address these issues alongside the change in configuration struct this weekend and update this PR.

@smf8
Copy link
Contributor Author

smf8 commented Aug 2, 2024

Hi again,

One way would be to add support for multiple URLs for the regular redis client as well. It could just try the provided connection infos in order and fail if neither one worked. That way one config could be used for all three implementations.

Won't this break the syntax for old users? (changing fields of regular redis Config struct) since the raw config formats (e.g. json, yaml, env, etc.) used for serde serialize/deserialize differ for Option<String> and Option<Vec<String>>?

@bikeshedder
Copy link
Owner

Won't this break the syntax for old users? (changing fields of regular redis Config struct) since the raw config formats (e.g. json, yaml, env, etc.) used for serde serialize/deserialize differ for Option<String> and Option<Vec<String>>?

True. That's why we keep a changelog and document changes like that. The change should be made in a way that either doesn't break existing configurations or breaks properly. By "breaking properly" I mean the user should be made aware that the configuration code returns an error if the old format is still being used rather than just ignoring the old values. If this is not feasible it could also be added as a deprecated option that only returns an error if both variants are being used.

This is nothing critical though. Feel free to duplicate the config structures for now and we'll clean this up at a later point. It's not a blocking issue with the code.

@smf8
Copy link
Contributor Author

smf8 commented Aug 29, 2024

Hi, I've tried fixing the CI pipeline. As for the config comment, I'll try sending another PR for that if that's OK.

@bikeshedder bikeshedder merged commit 70ab124 into bikeshedder:master Aug 29, 2024
64 checks passed
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.

2 participants