New Wrapper for cluster as in ISSUE #113 #128
Conversation
fix using {} to replace [] - untested
add queue_cluster_test.go
| queueReadyTemplate = "rmq::queue::[{queue}]::ready" // List of deliveries in that {queue} (right is first and oldest, left is last and youngest) | ||
| queueRejectedTemplate = "rmq::queue::[{queue}]::rejected" // List of rejected deliveries from that {queue} | ||
| queueReadyTemplate = "rmq::queue::{{queue}}::ready" // List of deliveries in that {queue} (right is first and oldest, left is last and youngest) | ||
| queueRejectedTemplate = "rmq::queue::{{queue}}::rejected" // List of rejected deliveries from that {queue} |
There was a problem hiding this comment.
This is a breaking change which is hard to deal with when using a setup which relies on [] (like we do internally). Do you think you could add a way to have {} by default, but switch back to using [] with some sort of flag? 🤔
There was a problem hiding this comment.
Yeah sure, I will see what I can do about this one
There was a problem hiding this comment.
@wellle Might know where I can find the setup which relies on [] ? Thanks
There was a problem hiding this comment.
Actually I found it already. We use this feature internally: twitter/twemproxy@f08088f
In there they use hash_tag: "{}" to demonstrate.
In our specific setup we use it like this instead: hash_tag: "[]".
So ideally we would be able to support both of these use cases (with {} being the default).
There was a problem hiding this comment.
Here's an idea of how it could be done. I don't like how it uses a global variable, so please let me know if you see a better approach. 🙏
-
Add a global
rmq.HashTag = "{}". This allows users (like us) to setrmq.HashTag = "[]"before opening an rmq connection. -
Then in
newQueue()we currently have code like this:readyKey := strings.Replace(queueReadyTemplate, phQueue, name, 1)
Let's replace that like this:
readyKey := queueReadyKey(name)
So in
redis_keys.gowe would add these functions which take thermq.HashTagvalue into account. For example it may look like this (untested):func queueReadyKey(queueName string) string { hashTagOpen, hashTagClose := rmq.HashTag[0], rmq.HashTag[1] return fmt.Sprintf("rmq::queue::%s%s%s::ready", hashTagOpen, queueName, hashTagClose) }
Do you think you could try this for the 4 keys which use the {queue} placeholder?
| return OpenConnectionWithRmqRedisClient(tag, RedisWrapper{redisClient}, errChan) | ||
| // OpenConnectionWithRedisClusterClient opens and returns a new connection for a cluster redis | ||
| func OpenConnectionWithRedisClusterClient(tag string, clusterClient *redis.ClusterClient, errChan chan<- error) (Connection, error) { | ||
| return OpenConnectionWithRmqRedisClient(tag, RedisClusterWrapper{clusterClient}, errChan) |
There was a problem hiding this comment.
Have you tried using the existing wrapper with the cluster client?
| return OpenConnectionWithRmqRedisClient(tag, RedisClusterWrapper{clusterClient}, errChan) | |
| return OpenConnectionWithRmqRedisClient(tag, RedisSingleWrapper{clusterClient}, errChan) |
When I compare these files
https://github.com/adjust/rmq/blob/3113601ad47836e110cd3201f160b69c172f9770/redis_wrapper_single.go
https://github.com/adjust/rmq/blob/3113601ad47836e110cd3201f160b69c172f9770/redis_wrapper_cluster.go
it looks like the cluster client has exactly the same functions, so I wonder if it might actually implement the redis.Cmdable interface, in which case we should be able to still use the existing wrapper instead of having the need for two different (but almost identical) wrapper implementations.
There was a problem hiding this comment.
Yes I have tried, works fine with me 💯
There was a problem hiding this comment.
so I wonder if it might actually implement the redis.Cmdable interface
This also confuses me, the redis.Client does implement the redis.Cmdable, but, redis.ClusterClient does not.
I have checked the wrapper functions, which is common, and my team also wrapped same functions both for cluster and single redis.
It also needs some work to do to make the code more elegent, such as making new interface to fit the two type of redis clients, I will see what I can do in the future, thanks for pointing out! 👍
There was a problem hiding this comment.
The redis.ClusterClient has implementd Cmdable already, so this wrapper might be not necessary
There was a problem hiding this comment.
Thanks for jumping in here! 🙌
Do you think you can take this branch and remove this wrapper again to see if it works without it? Feel free to open a new PR with the updated branch then. Thanks again!
|
Still needs working, will make PR later |
|
How is it going with this PR? I got an issue in which I cannot use the rmq in cluster mode. |
|
@zcqzcg: Are you still working on this? If not, maybe someone else can pick up this PR and try to address the review comments above 🙏 |
SoSorry I haven't have enough time to work with this repo for now. Please if someone could take over |
|
Thanks for the update! Any takers? 🙏 |
|
Seeing the conversation in #113 I feel like this may have been resolved by the changes made related to Redis Cluster. I'm closing it as I'm not sure if this problem still persists. Please feel free to reopen if it does ✌️ |
Hi team,
Thanks to the authors to provide such a nice and effective rmq for Gophers!
I'm using this packge and have to work with a redis cluster, so I forked and added a new RedisClusterWrapper for the cluster to fit in.
*redis.Optionwhich could allow users to connect their redis more freelyPlease feel free to modify/update these commits
====================
And a big thanks to ISSUE #113 , @obh provided a direction to the cluster path
Best,
David