[Cache] Ensured that redis adapter can use multiple redis sentinel hosts #47003
+22
−14
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
|
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. |
nicolas-grekas
changed the title
nicolas-grekas
force-pushed
the
46998-cache-comp-redis-sentinel-multihost-fix
branch
from
7236258
to
39998d3
Compare
|
Good catch, thanks @warslett. |
This was referenced Jul 29, 2022
Merged
Merged
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.
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.