Merged
Conversation
fix using {} to replace [] - untested
add queue_cluster_test.go welle: Remove hardcoded connection options.
Add new function OpenConnectionWithOptions() for the new approach.
Rename RedisSingleWrapper back to RedisWrapper
To allow opening RMQ connections which use the Redis hash tags {}
instead of []. This is required to make rmq work with Redis clusters.
This commit also reverts the behavior of all other OpenConnection[...]
functions to behave as before by still using [] instead of {}.
This switch is done by using different Redis key templates. For example
instead of
rmq::connection::{connection}::queue::[{queue}]::consumers
we would use
rmq::connection::{connection}::queue::{{queue}}::consumers
when using OpenClusterConnection()
Lines Of Code
|
Go API Changes# github.com/adjust/rmq/v5 ## compatible changes OpenClusterConnection: added OpenConnectionWithRedisOptions: added # summary Inferred base version: v5.1.3 Suggested version: v5.2.0 |
Benchmark ResultBenchmark diff with base branchBenchmark result |
vearutop
force-pushed
the
113-redis-cluster-support
branch
from
445ae71 to
e8454e6
Compare
Unit Test Coveragetotal: (statements) 71.4% Coverage of changed lines
Coverage diff with base branch
|
vearutop
approved these changes
Jul 18, 2023
Contributor
vearutop
left a comment
vearutop
left a comment
There was a problem hiding this comment.
I've updated CI setup to use the mentioned create-cluster script instead of a dockerized service.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Close #113.
This PR adds support for using rmq with a Redis cluster by allowing to use the Redis hash tags
{}instead of the characters[]we used before. We used[]before to allow having certain keys live on the same Redis node using nutcracker, in the same way{}is now used by Redis cluster.This is a continuation of #128. I took the original commits (with slight modifications) and then added a few more to:
OpenConnection()functions, make sure their behavior doesn't change compared to master.OpenClusterConnection()function which uses the different Redis hash keys{}instead of[].Note: I managed to run the tests locally by following https://github.com/redis/redis/tree/unstable/utils/create-cluster and then running the tests like this:
But in the current state they are not easy to run locally (you need a Redis cluster) and I expect them to fail in CI too. We should try to improve/fix that. Maybe there's a way to set up a working Redis cluster with
miniredis(which is already used in some tests)?