Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Cache] Improve RedisTagAwareAdapter invalidation logic & requirements#33461
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Cache] Improve RedisTagAwareAdapter invalidation logic & requirements#33461
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas left a comment
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.
That's cool!
A hypothetical future step could be to schedule the cleanups using Messenger :)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
andrerom commentedSep 5, 2019
Some setbacks here, clients arenot so complete:
These can potentially be fixed by instead using SMEMBERS + skipping SSCAN/SMEMBERS on tagId if RENAME returned 0(not found in this case). More of a Server issue:
This one I'm unsure of, should we try to make sure we RENAME within same node? Suggestions@nicolas-grekas /@berezuev? |
nicolas-grekas commentedSep 5, 2019
Can't we rename to a key that will be guaranteed on the same node, using the special syntax they support for this? |
andrerom commentedSep 5, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Do you happen to remember where that is described? Thought about that too, but didn't know where to look. |
nicolas-grekas commentedSep 6, 2019
Sure, seehttps://redis.io/topics/cluster-spec, the section about "hash tags" |
stof commentedSep 6, 2019
is it possible to have |
This comment has been minimized.
This comment has been minimized.
nicolas-grekas commentedSep 6, 2019
The alternative could be to talk directly to the corresponding node in the cluster. That's possible AFAIK. |
This comment has been minimized.
This comment has been minimized.
nicolas-grekas commentedSep 6, 2019
I think there is a function for that :) |
e264119 toe2c430bCompareandrerom commentedSep 22, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
RedisCluster failure was fixed inbe04079. I did not find a simple way to deal with AppVeyor failure seems unrelated(HttpClient\Tests\NativeHttpClientTest::testNotATimeout), so rebased and moving this to review. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas left a comment
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.
please rebase, you've been caught by fabbot :)
Uh oh!
There was an error while loading.Please reload this page.
cbc72b7 tof9e47baCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
andrerom commentedSep 26, 2019
last commit 2017... |
nicolas-grekas commentedSep 26, 2019
Another reason to throw and encourage using phpredis... |
fabpot commentedSep 27, 2019
Mergeable? |
andrerom commentedSep 27, 2019
IMHO Yes, we can do the improvements as a follow up, it will anyway need some investigation and testing. But it's entirely up to you two. |
nicolas-grekas commentedSep 27, 2019
I'd prefer doing it in this PR, depending on your availability to do it :) |
db80b9d tob41526eComparenicolas-grekas commentedOct 7, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hello, I just push-forced this here: Should fix the last comment. |
Uh oh!
There was an error while loading.Please reload this page.
e28a53e to28948eeCompare
andrerom left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
+1 Looks good to me 👍
Further improvements here depends on future improvements in Predis if anyone is up for the tasks:
- Exception on rename on missing key(should align with phpredis: false response when missing)
- Lack of api to find slot of key when using RedisCluster class(should also be added to ClusterInterface, based on implementation in PredisCluster class)
28948ee to093a3b4Comparenicolas-grekas commentedOct 8, 2019
Actually, I just discovered this is already possible! PR updated to use pipelining.
that's a bit more involving, as that'd require implementing the logic to react to |
67d569d to5ece961Compare5ece961 to3d38c58Comparenicolas-grekas commentedOct 8, 2019
Thank you@andrerom. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Cache/Tests/Adapter/PredisTagAwareRedisClusterAdapterTest.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Changes logic of invalidation in RedisTagAwareAdapter in order to:
Positive side effects of no longer using sPOP: