Add redis sentinel support #107
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jaymell
force-pushed
the
redis-sentinel
branch
2 times, most recently
from
55f3bcc
to
1feda18
Compare
|
Is the breaking change only when sentinel is enabled? |
No. We could potentially add a field that's dependent on the feature, but I think a better approach is to stop making config fields public and instead switch to builders. This will cause some issues downstream with Bridge IIRC, but that's something that I'm guessing should be fixable. |
There are a few breaking changes made to support this. Most notably, the an optional `SentinelConfig` struct has been added to the RedisConfig object. It seems like obfuscating the fields here might be a good idea, but that's not how we've been rolling thus far with the config objects. Also, the `RedisConnection` trait has been redone to take a `RedisConfig` instead of a DSN in order to build the connection. Sentinel needs it owing to the large number of potential config params needed for Sentinel, and I think this is probably a better approach anyway. The base redis tests (well, most of them) have also been updated to support testing against either base Redis or Sentinel. The `rstest` crate has been added in order to be able to parameterize the tests sanely. We also use this crate for tests in `redis-rs`, so I think it's a no-brainer to add here.
jaymell
force-pushed
the
redis-sentinel
branch
from
1feda18
to
292d4d7
Compare
svix-onelson
approved these changes
Oct 4, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are a few breaking changes made to support this.
Most notably, the an optional
SentinelConfigstructhas been added to the RedisConfig object. It seems like
obfuscating the fields here might be a good idea, but
that's not how we've been rolling thus far with the
config objects.
Also, the
RedisConnectiontrait has been redoneto take a
RedisConfiginstead of a DSN in orderto build the connection. Sentinel needs it owing
to the large number of potential config params needed
for Sentinel, and I think this is probably a better
approach anyway.
The base redis tests (well, most of them) have also
been updated to support testing against either
base Redis or Sentinel. The
rstestcrate has beenadded in order to be able to parameterize the tests
sanely. We also use this crate for tests in
redis-rs,so I think it's a no-brainer to add here.