Skip to content
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

[Cache] Improve RedisTagAwareAdapter invalidation logic & requirements #33461

Merged

Conversation

@andrerom
Copy link
Contributor

commented Sep 4, 2019

Q A
Branch? 4.4
Bug fix? yes, and improvment
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT
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 more and 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)
Copy link
Member

left a comment

That's cool!
A hypothetical future step could be to schedule the cleanups using Messenger :)

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

Some setbacks here, clients are not 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 and RedisCluster 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

This comment has been minimized.

Copy link
Member

commented Sep 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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

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-grekas nicolas-grekas added this to the next milestone Sep 6, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

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

@stof

This comment has been minimized.

Copy link
Member

commented Sep 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 was marked as outdated.

Copy link
Contributor Author

commented Sep 6, 2019

Thanks for the hints 👍

However still getting RedisCluster::rename(): Keys don't hash to the same slot. Could be it's phpredis which forbids it. Like Predis Cluster client does with a Predis\NotSupportedException: Cannot use 'RENAME' with redis-cluster.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

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

@andrerom

This comment was marked as outdated.

Copy link
Contributor Author

commented Sep 6, 2019

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

yes, I'll "just" need to figure out which node the key is on, and it might also work on Predis. However that will be topic for after vacation then 🌴😏

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

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

@andrerom andrerom force-pushed the andrerom:improved_tagaware_redis_adapter branch 3 times, most recently from e264119 to e2c430b Sep 22, 2019
@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

RedisCluster failure was fixed in be04079. I did not find a simple way to deal with RENAME 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.

@andrerom andrerom marked this pull request as ready for review Sep 22, 2019
Copy link
Member

left a comment

please rebase, you've been caught by fabbot :)

@andrerom andrerom force-pushed the andrerom:improved_tagaware_redis_adapter branch from cbc72b7 to f9e47ba Sep 25, 2019
@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

@nicolas-grekas Be aware of one new issue introduced here, even if we log warnings about it:

  • With Predis cluster there is now a race condition issue where tags added to set between it being read with SMEMBERS and whole set being Deleted at the end will be lost
  • So you could argue that we should keep SPOP logic for Predis Cluster as it does not have race condition issue at least. It would still mean we don't require phpredis 3.1.3, but it will then continue to require 3.2 server for Predis Cluster setups
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

I miss some background here: how does phpredis do to support RENAME on cluster? Can't we mimic this with Predis, e.g calculating the node then doing the RENAME on its specific connection?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

Or should we throw an exception when this adapter is used with a Predis cluster?

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

I miss some background here: how does phpredis do to support RENAME on cluster?

Predis Cluster aggregates throws on it from PHP code no matter what, phpredis only if there is a cross slot response which we avoid by using the {key}hash key form.

Can't we mimic this with Predis, e.g calculating the node then doing the RENAME on its specific connection?

On PredisCluster Connection we probably easily can: https://github.com/nrk/predis/blob/111d100ee389d624036b46b35ed0c9ac59c71313/src/Connection/Aggregate/PredisCluster.php#L159

On (Predis) RedisCluster Connection there is no such method, and while there is ->getClusterStrategy()->getSlotByKey($key), it's a bit unclear where to go from there.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

On PredisCluster Connection we probably easily can

let's do it?

On (Predis) RedisCluster Connection

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

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

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

last commit 2017...

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

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

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

Mergeable?

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Sep 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

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

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

@nicolas-grekas nicolas-grekas force-pushed the andrerom:improved_tagaware_redis_adapter branch from db80b9d to b41526e Oct 7, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

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

Should fix the last comment.

@nicolas-grekas nicolas-grekas force-pushed the andrerom:improved_tagaware_redis_adapter branch 3 times, most recently from e28a53e to 28948ee Oct 7, 2019
Copy link
Contributor Author

left a comment

+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-grekas nicolas-grekas force-pushed the andrerom:improved_tagaware_redis_adapter branch from 28948ee to 093a3b4 Oct 8, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

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 to MOVE responses. Should be handled at the Predis level instead, as phpredis does.

@nicolas-grekas nicolas-grekas force-pushed the andrerom:improved_tagaware_redis_adapter branch 2 times, most recently from 67d569d to 5ece961 Oct 8, 2019
@nicolas-grekas nicolas-grekas force-pushed the andrerom:improved_tagaware_redis_adapter branch from 5ece961 to 3d38c58 Oct 8, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

Thank you @andrerom.

nicolas-grekas added a commit that referenced this pull request Oct 8, 2019
…c & requirements (andrerom)

This PR was merged into the 4.4 branch.

Discussion
----------

[Cache] Improve RedisTagAwareAdapter invalidation logic & requirements

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes, _and improvment_
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT
| 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 more and 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)_

Commits
-------

3d38c58 [Cache] Improve RedisTagAwareAdapter invalidation logic & requirements
@nicolas-grekas nicolas-grekas merged commit 3d38c58 into symfony:4.4 Oct 8, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@andrerom andrerom deleted the andrerom:improved_tagaware_redis_adapter branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.