[HttpFoundation][Cache][Messenger] Replace redis "DEL" commands with "UNLINK" #37993
Conversation
|
The method is also used in src/Symfony/Component/Cache/Traits/RedisTrait.php and in src/Symfony/Component/Messenger/Bridge/Redis/Transport/Connection.php Would you advise updating these also? If yes, can you please update the PR? Can you please add a note in the UPGRADE-5.2 file, in each component's section? |
|
@jonashrem Still interested in finishing this PR? |
|
@fabpot yes, I was just a bit busy the last days. I still plan looking into this. |
|
/cc @andrerom btw, any opinion about the proposed bump to Redis >= 4? |
|
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. |
|
I replaced the other occurrences. 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: 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. |
|
@jonashrem friendly ping. Are you OK with my previous comment? |
yes, that's fine. I will look into this further if it's possible to set this manually. |
67b8be2 to
ffc134a
Compare
There was a problem hiding this comment.
I updated the code to fallback to DEL when UNLINK is not available.
I also submitted predis/predis#666 to add UNLINK to Predis.
Ready on my side.
|
oh, that's a very good idea. In this case we don't even loose backwards compatibility with redis 3.x. Very nice. So for my understanding when the Integration tests run through there is nothing left to do here? |
ffc134a to
ae9ae17
Compare
Correct, there should be nothing else to do here. Now waiting for the CI to be green for the components. |
ae9ae17 to
91a4452
Compare
|
Thank you @jonashrem. |
|
Thanks, as well. And sorry you had to do so much in the end by yourself. |
This PR replaces DEL Command with UNLINK command.
Details about UNLINK can be found here https://redis.io/commands/unlink .
As noticed here redis/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.
See also http://antirez.com/news/93 and redis/redis#1748
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.