-
Notifications
You must be signed in to change notification settings - Fork 11k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[5.1] Fix for deleting tag keys in Redis cluster #17792
Conversation
This sounds like a performance nightmare? |
You may want to backport #16568. It fixes the same issue, in a better way, using chunks. |
In order to implement multi-key operations in Redis Cluster, it is needed to implement "hash tags" as explained by Redis Docs at https://redis.io/topics/cluster-spec#keys-hash-tags it.
@taylorotwell sure it seems to be @vlakoff thanks but it isn't the solution to the issue explained in #17713 I contacted @nrk, Predis author, who gave me some important informations and linked me to: I just made some changes on I made some tests on my environments and it seems to work fine. |
@@ -146,7 +146,7 @@ protected function deleteValues($referenceKey) | |||
$values = array_unique($this->store->connection()->smembers($referenceKey)); | |||
|
|||
if (count($values) > 0) { | |||
call_user_func_array([$this->store->connection(), 'del'], $values); | |||
call_user_func_array([$this->store->connection(), 'del'], [$value]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you forgot to undo this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Yes, that change could work but honestly I wouldn't want to make it on 5.1. It would be a breaking change. It would be something to be considered for 5.5 perhaps. |
@taylorotwell It would break stored tagged keys because changing their prefix they won't be recognized. All this in a non-persistent caching system. But would be an important fix for changes occurred with 5.1.30 which bring good implementations but completely compromise the use of tagged keys on Redis cluster (strangely it works before). Right now I can't upgrade an LTS above 5.1.29 because of broken changes... |
@taylorotwell someone already did a breaking change on 5.1.30, this is merely a fix. |
When Predis is connected to a cluster, you could just group keys by the slot they are associated to by using something along these lines. You will still end up having multiple |
Actual Laravel implementation of tags is not using the curly braces, so instead of {tag1}:abc key name is tag1:abc and this is causing the problem |
@FabryB yes this is the main issue @nrk solution's heads toward group keys and launch |
@FabryB as I've clarified in my comment my solution doesn't require the use of curly braces, the hash-tags (in redis lingo it's the portion of a key between curly braces that is used to compute its hash) in that snippet are just there to easily reproduce the case of more than one key associated to the same slot. |
This is a fix proposal for the issue documented in #17713.
I would ask for @mstephens review to get his expertise and about his last implementations on
Illuminate\Cache\RedisTaggedCache
.