NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[HttpFoundation][Cache][Messenger] Replace redis "DEL" commands with "UNLINK"#37993

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

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:masterfromjonashrem:patch-1

Sep 27, 2020

Merged

[HttpFoundation][Cache][Messenger] Replace redis "DEL" commands with "UNLINK"#37993

nicolas-grekas merged 1 commit intosymfony:masterfromjonashrem:patch-1

Sep 27, 2020

Conversation

Copy link

Contributor

jonashrem commentedAug 30, 2020•
edited by nicolas-grekas
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	no
Deprecations?	no
Tickets
License	MIT
Doc PR

This PR replaces DEL Command with UNLINK command.

Details about UNLINK can be found herehttps://redis.io/commands/unlink .

As noticed hereredis/redis#1748 (comment) this will not change the behavior.

As there is no check of Redis version included, it will break Redis Versions < 4.0 through.

As Redis 4.0 was released on 2017-07-14 and Redis 3 went EOL with the Redis 5 on 2018-10-17, I don't expect this being an issue.

With redis 4.0 there is a new unlink command intruded (https://redis.io/commands/unlink ) with is a smarter version of del() as it deletes keys in O(1) and frees the memory in background.

This won't change the behavior but only the memory usage as clarified here:redis/redis#1748 (comment) which shouldn't be an issue with most Magento instances

With this, I recommend replacing DEL operations with UNLINK operations.

carsonbot added the Status: Needs Review label

Aug 30, 2020

nicolas-grekas added this to thenext milestone

Aug 31, 2020

Copy link

Member

nicolas-grekas commentedAug 31, 2020

The method is also used insrc/Symfony/Component/Cache/Traits/RedisTrait.php and insrc/Symfony/Component/Messenger/Bridge/Redis/Transport/Connection.php

Would you advise updating these also? If yes, can you please update the PR?
I'm fine with dropping support for Redis 3 on 5.2.

Can you please add a note in the UPGRADE-5.2 file, in each component's section?

Copy link

Member

fabpot commentedSep 6, 2020

@jonashrem Still interested in finishing this PR?

Copy link

ContributorAuthor

jonashrem commentedSep 6, 2020

@fabpot yes, I was just a bit busy the last days. I still plan looking into this.

javiereguiluz added the HttpFoundation label

Sep 8, 2020

nicolas-grekas added Cache Messenger labels

Sep 8, 2020

nicolas-grekas changed the title~~replace redis "DEL" Commands with "UNLINK"~~[HttpFoundation][Cache][Messenger]replace redis "DEL" Commands with "UNLINK"

Sep 8, 2020

nicolas-grekas changed the title~~[HttpFoundation][Cache][Messenger]replace redis "DEL" Commands with "UNLINK"~~[HttpFoundation][Cache][Messenger] Replace redis "DEL" Commands with "UNLINK"

Sep 8, 2020

nicolas-grekas changed the title~~[HttpFoundation][Cache][Messenger] Replace redis "DEL" Commands with "UNLINK"~~[HttpFoundation][Cache][Messenger] Replace redis "DEL" commands with "UNLINK"

Sep 8, 2020

Copy link

Member

nicolas-grekas commentedSep 9, 2020

/cc@andrerom btw, any opinion about the proposed bump to Redis >= 4?

Copy link

andrerom commentedSep 11, 2020

For master that should be ok from my POV 👍

Make sure to mention it clearly in release notes though, not everyone expect bumps in requirements in minor releases.

jonashrem requested a review fromsroze as acode owner

September 13, 2020 12:25

Copy link

ContributorAuthor

jonashrem commentedSep 13, 2020

I replaced the other occurrences.

For the failing auto-tests, is it possible the test instance is still using redis 3?

Copy link

Member

nicolas-grekas commentedSep 14, 2020•
edited
Loading

For the failing auto-tests, is it possible the test instance is still using redis 3?

The CI runs Redis 6 apparently, but the failures are helpful:Predis doesn't know about the unlink command. Looking at the code, that's the case:https://github.com/predis/predis/tree/main/src/Command/Redis

Would you like to send a PR there to add the command?

Also, can't we add a command "manually" on our side in order to teach Predis about it?

Last note here: don't forget to add a note in the UPGRADE-5.2 file, in each component's section.

Thanks.