Make Relay work with redis cluster #1397
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
dorrogeray
force-pushed
the
fix-relay-working-with-redis-cluster
branch
from
246e11b to
96b9bc5
Compare
|
Thanks! Can you re-enable some of the cluster tests to ensure this works? |
dorrogeray
commented
Sep 8, 2023
dorrogeray
force-pushed
the
fix-relay-working-with-redis-cluster
branch
4 times, most recently
from
b1216bd to
63401d0
Compare
dorrogeray
force-pushed
the
fix-relay-working-with-redis-cluster
branch
5 times, most recently
from
75f2d84 to
7fbdaf8
Compare
dorrogeray
force-pushed
the
fix-relay-working-with-redis-cluster
branch
from
7fbdaf8 to
8ddce5f
Compare
|
@tillkruss ready for review |
tillkruss
reviewed
Sep 11, 2023
tillkruss
previously approved these changes
Sep 11, 2023
|
Thanks for the work @dorrogeray on this. I've pushed a few formatting changes. BTW, we're currently working on full |
|
Can we get your review @vladvildanov? |
tillkruss
reviewed
Sep 11, 2023
tillkruss
reviewed
Sep 11, 2023
szepeviktor
reviewed
Sep 11, 2023
tillkruss
reviewed
Sep 11, 2023
dorrogeray
force-pushed
the
fix-relay-working-with-redis-cluster
branch
from
e507535 to
97a9754
Compare
|
@dorrogeray: The PR looks great. The only concern that I have, which isn't directly related is Would you mind seeing if we can run the one simple Redis command test case against cluster? |
dorrogeray
force-pushed
the
fix-relay-working-with-redis-cluster
branch
2 times, most recently
from
4003f34 to
b4d981f
Compare
|
@tillkruss I have added some cluster tests. The |
tillkruss
requested review from
vladvildanov and
chayim
and removed request for
vladvildanov
tillkruss
approved these changes
Sep 11, 2023
- Ensure connection is open when RelayConnection::getIdentifier is called - Return error responses instead of throwing them in RelayConnection::executeCommand so that RedisCluster::executeCommand can intercept MOVED errors and handle them with RedisCluster::onErrorResponse - Make parsing of connection id in RedisCluster::onMovedResponse more resilient by culling off details from the exception message - this is required because relay exception message format is "IP:port (details about exception)"
tillkruss
force-pushed
the
fix-relay-working-with-redis-cluster
branch
from
b4d981f to
f8f4b14
Compare
vladvildanov
pushed a commit
to vladvildanov/predis
that referenced
this pull request
Jan 13, 2025
tillkruss
pushed a commit
that referenced
this pull request
Jan 14, 2025
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.
Currently, when predis client is configured to use a combination of Relay and Redis Cluster, it does not work correctly when redis replies with
MOVED. Following set of fixes should get it working:CC: @tillkruss