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
Flush Redis prevnext
cache when clearing civicrm_cache
#27113
Conversation
Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷 Introduction for new contributors
Quick links for reviewers |
prevnext
cache when clearing civicrm_cache
I'm entirely comfortable with the Redis cache being cleared at the same time. |
// Clear the Redis prev-next cache, if there is one. | ||
// Since we truncated the civicrm_cache table it is logical to also remove | ||
// the same from the Redis cache here. | ||
\Civi::service('prevnext')->deleteItem(); |
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.
I think the comment is correct - clearing civicrm_cache
in the normal case implies that you can clear prevnext in the Redis case. It should be said that this isn't really parity between SQL and Redis. For that, it would need something like:
- When flushing, delete all Redis keys based on the site-prefix.
- Fix the oddball Redis keys which lack site-prefix. (Like, ahem, prevnext...)
(Ok, for completeness, another way to reach parity is to rework the prefixes -- instead of literal site-prefix, map each prefix to redis database id. Then FLUSHDB
or SWAPDB
become options. But that's not easier.)
Anyway, it certainly sounds more conservative (safer) to just flush the part you care about. And in that approach, I'm quite glad there's a comment to explain the thinking. (These cache-clear functions are a bit of maze, and you often have to scratch your head at each junction of the maze.)
I think it's functionally safe to merge. Flagging "Merge ready" (in case you want to mull any further). |
ok this has been merge ready long enough |
Overview
Flush Redis
prevnext
cache when clearingcivicrm_cache
Before
There is actually nowhere in CiviCRM where the Redis Prev-next cache is flushed. It is possible to flush using eval - ie
but in normal operations it is never flushed
After
It would be flushed in the same instances as the
civicrm-cache
table is clearedTechnical Details
Once we get Redis to start expiring it won't be so necessary to flush the cache - but it should be possible to flush it 'somehow' via the api/ I'm a bit on the fence between this & something a bit more opt-in.
Comments