[Cache] Ensured that redis adapter can use multiple redis sentinel hosts #47003
Conversation
This file contains hidden or 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
|
okay I found the test: RedisAdapterSentinelTest will update |
|
I've added multiple sentinel hosts to integration-tests.yml which makes it configure more like a real sentinel setup and I believe would have caught this bug (even though each provided host is actually the same address). I've also fixed the failing test due to the change in exception message. |
7236258 to
39998d3
Compare
|
Good catch, thanks @warslett. |
This was referenced Jul 29, 2022
Merged
Merged
Merged
nicolas-grekas
added a commit
that referenced
this pull request
Sep 20, 2023
…sts (digilist) This PR was merged into the 6.4 branch. Discussion ---------- [Messenger] Add support for multiple Redis Sentinel hosts | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | n/a | License | MIT | Doc PR | n/a Similar to #47003 which added support for multiple Redis Sentinel hosts for the Cache component, this PR adds support for multiple Sentinel hosts for the Messenger component. This PR is inspired by the implementation in the cache component and works very similar. A DSN could look like this: `redis:?host[localhost:26377]&host[localhost:26379]&sentinel_master=db`. I changed the Sentinel host environment variable for the ingegration to an invalid host at. As a result I noticed that Relay also fails in such case and so I expanded my earlier changes from #51598 to also ignore unreachable hosts with the Relay extension. Commits ------- 3380518 [Messenger] Add support for multiple Redis Sentinel hosts
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.
This PR ensures that RedisAdapter for Symfony Cache component works as expected when a redis_sentinel argument is provided with multiple redis sentinel hosts. Previously if using the redis php extension and multiple hosts were provided RedisTrait::createConnection would ignore the redis_sentinel parameter and instead return a RedisArray object which would not work because it would be an array of sentinel hosts rather than redis hosts.
This PR introduces the correct behaviour where by RedisTrait::createConnection will loop through each of the provided sentinel hosts trying each one to get the address of the mast redis instance. If none of the provided hosts can return the address of a master instance it will throw an exception.
I'm not too sure how to test this behaviour. There doesn't appear to be any tests for redis sentinel behaviour in place currently. I have tested manually both with a multi redis sentinel setup and with a single standard redis instance and the code now functions as expected in both scenarios.