search

bikeshedder / deadpool

Dead simple pool implementation for rust with async-await
Apache License 2.0
1.08k stars 137 forks source link

Add redis sentinel connection pool #339

Closed smf8 closed 2 months ago

smf8 commented 4 months ago

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 commented 4 months ago

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. :thinking:

smf8 commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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.