Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Conversation

@andrerom
Copy link

@andreromandrerom commentedSep 4, 2019
edited
Loading

QA
Branch?4.4
Bug fix?yes,and improvment
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
LicenseMIT
Doc PR

Changes logic of invalidation in RedisTagAwareAdapter in order to:

  • Delete the tag key on invalidation =>avoiding possible left behind empty tag keys that Redis is not allowed to evict, gradually consuming slightly more memory

Positive side effects of no longer using sPOP:

  • Lowered requirements to Redis 2.8, and no specific version constraint for phpredis
  • Lift limitation of 2 billion keys per tag(Now only limited by Redis Set datatype: 4 billion)

adamwojs and nicolas-grekas reacted with thumbs up emoji
Copy link
Member

@nicolas-grekasnicolas-grekas left a 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 :)

@andrerom
Copy link
Author

Some setbacks here, clients arenot so complete:

  • RedisArray does not support SSCAN, you'll get:
    Parameter 2 to Redis::sscan() expected to be a reference, value given
  • Predis alos have limited SSCAN and gives the following if key does not exists:
    Predis\Response\ServerException: ERR no such key

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:

  • Cluster on Predis andRedisCluster does not support RENAME:
    RedisCluster::rename(): Keys don't hash to the same slot

This one I'm unsure of, should we try to make sure we RENAME within same node?

Suggestions@nicolas-grekas /@berezuev?

@nicolas-grekas
Copy link
Member

Can't we rename to a key that will be guaranteed on the same node, using the special syntax they support for this?

@andrerom
Copy link
Author

andrerom commentedSep 5, 2019
edited
Loading

using the special syntax they support for this?

Do you happen to remember where that is described? Thought about that too, but didn't know where to look.

@nicolas-grekasnicolas-grekas added this to thenext milestoneSep 6, 2019
@nicolas-grekas
Copy link
Member

Sure, seehttps://redis.io/topics/cluster-spec, the section about "hash tags"

@stof
Copy link
Member

stof commentedSep 6, 2019

is it possible to have{ and} in our normal tagId Redis keys ? If yes, we won't be able to use hash tags to achieve our goal.
Otherwise, this can indeed by used, making the temporary tag id being'{'.$tagId.'}'.$randomSuffix instead of$tagId.':'.$randomSuffix.

@andrerom

This comment has been minimized.

@nicolas-grekas
Copy link
Member

The alternative could be to talk directly to the corresponding node in the cluster. That's possible AFAIK.

@andrerom

This comment has been minimized.

@nicolas-grekas
Copy link
Member

I think there is a function for that :)
Happy 🌴 !

andrerom reacted with hooray emoji

@andreromandreromforce-pushed theimproved_tagaware_redis_adapter branch 3 times, most recently frome264119 toe2c430bCompareSeptember 22, 2019 18:46
@andrerom
Copy link
Author

andrerom commentedSep 22, 2019
edited
Loading

RedisCluster failure was fixed inbe04079. I did not find a simple way to deal withRENAME on Predis across RedisCluster & PredisCluster so left that as-is, could instead use SPOP there if prefered.

AppVeyor failure seems unrelated(HttpClient\Tests\NativeHttpClientTest::testNotATimeout), so rebased and moving this to review.

@andreromandrerom marked this pull request as ready for reviewSeptember 22, 2019 19:29
Copy link
Member

@nicolas-grekasnicolas-grekas left a 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 :)

@andreromandreromforce-pushed theimproved_tagaware_redis_adapter branch fromcbc72b7 tof9e47baCompareSeptember 25, 2019 16:08
@andrerom
Copy link
Author

let's throw for now in this situation and open an issue on Predis?

last commit 2017...

nicolas-grekas reacted with confused emoji

@nicolas-grekas
Copy link
Member

Another reason to throw and encourage using phpredis...
Let's still create the issue for the record...

@fabpot
Copy link
Member

Mergeable?

@andrerom
Copy link
Author

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
Copy link
Member

I'd prefer doing it in this PR, depending on your availability to do it :)

andrerom reacted with thumbs up emoji

@nicolas-grekasnicolas-grekasforce-pushed theimproved_tagaware_redis_adapter branch fromdb80b9d tob41526eCompareOctober 7, 2019 16:27
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedOct 7, 2019
edited
Loading

Hello, I just push-forced this here:
https://github.com/symfony/symfony/compare/db80b9db0519ec73165b9bc2d57674993c7a3d5e..28948eeb8b

Should fix the last comment.

@nicolas-grekasnicolas-grekasforce-pushed theimproved_tagaware_redis_adapter branch 3 times, most recently frome28a53e to28948eeCompareOctober 7, 2019 16:58
Copy link
Author

@andreromandrerom left a comment
edited
Loading

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:

  1. Exception on rename on missing key(should align with phpredis: false response when missing)
  2. Lack of api to find slot of key when using RedisCluster class(should also be added to ClusterInterface, based on implementation in PredisCluster class)

@nicolas-grekasnicolas-grekasforce-pushed theimproved_tagaware_redis_adapter branch from28948ee to093a3b4CompareOctober 8, 2019 09:41
@nicolas-grekas
Copy link
Member

Exception on rename on missing key

Actually, I just discovered this is already possible! PR updated to use pipelining.

Lack of api to find slot of key when using RedisCluster

that's a bit more involving, as that'd require implementing the logic to react toMOVE responses. Should be handled at the Predis level instead, as phpredis does.

andrerom reacted with thumbs up emoji

@nicolas-grekasnicolas-grekasforce-pushed theimproved_tagaware_redis_adapter branch 2 times, most recently from67d569d to5ece961CompareOctober 8, 2019 09:53
@nicolas-grekasnicolas-grekasforce-pushed theimproved_tagaware_redis_adapter branch from5ece961 to3d38c58CompareOctober 8, 2019 10:09
@nicolas-grekas
Copy link
Member

Thank you@andrerom.

andrerom reacted with hooray emoji

@andreromandrerom deleted the improved_tagaware_redis_adapter branchOctober 8, 2019 10:46
@nicolas-grekasnicolas-grekas modified the milestones:next,4.4Oct 27, 2019
This was referencedNov 12, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@stofstofstof left review comments

+1 more reviewer

@berezuevberezuevberezuev left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

6 participants

@andrerom@nicolas-grekas@stof@fabpot@berezuev@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp