dev/core#4501 Redis - fix ttl on prev-next cache

[eileenmcnaughton]

Overview

Redis - fix ttl on prev-next cache

Before

The ttl is not being set on the prevnext keys when the cache in use is Redis. This can been seen by doing a search in the UI via basic search or advanced search (but not search kit) and then opening up redis-cli.

In redis-cli you can find the keys created using the keys command

The ttpl for those keys can be seen with the ttl command - ie

 ttl  "/prevnext/civicrm search CRMContactControllerSearch1v64jxfujuckw4g4o40kg8wcck0wscs4wskcwkck0g4gg48wws_8715/all"

Without this patch the ttl is always -1

After

Afterwards the ttl should have a value at all times - note that I testing searching along with selecting & unselecting specific contacts and deleting a contact from within the set

Technical Details

The expires was being set on the key before it was created - not doing anything. I could not find any evidence that it is possible to create an empty key & hence concluded we have to set expires first key-add. It seems redis-cli accepts expires as part of the set command and I think our php library supports that for the basic set - but not all the variants

Comments

tips

1. if using Redis on docker it is good to add redis-cli & redis-monitor to .bash_aliases like

alias redis-monitor='docker-compose --file /home/eileen/dev/fundraising-dev/docker-compose.yml exec queues redis-cli monitor'
alias redis-cli='docker-compose --file /home/eileen/dev/fundraising-dev/docker-compose.yml exec queues redis-cli'

2. if using Redis on a system with a password try something like this in .profile

export REDIS_PWD=totally_secret_password
alias redis-monitor='redis-cli -a $REDIS_PWD monitor'
alias redis-cli='redis-cli -a $REDIS_PWD'

3. you can count the keys that are on your system like this
   You can also count them like this ....

redis-cli keys *prevnext* | wc -l

4. if you have had Redis running for a while you may have a build up of keys from prev-next searches (we have 35k) - there is no api action to clear these but you can use eval (via cv or drush) to clear them out

 cv php:eval "\Civi::service('prevnext')->deleteItem();"

[civibot]

Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷

Introduction for new contributors

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers

• ➡️ Demo site for this PR
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

[eileenmcnaughton]

@seamuslee001 @johntwyman this is probably the more useful one - along with possibly #27113

[johntwyman]

Currently our Redis cache is reasonably small so I'm not seeing an absurdly high number of keys, as occasionally we go so far as to issue a FLUSHALL via redis-cli. In other words, the non-expiry of keys in our Redis cache eventually causes problems.

So I'm all for this change. Thanks @eileenmcnaughton

[eileenmcnaughton]

@johntwyman yes ' eventually' is probably fair - this has taken years to get to this point for us

[eileenmcnaughton]

@totten we have just deployed this in production & I can confirm that the ttl is now being set on the prevnext redis keys!

[totten]

@eileenmcnaughton That's pretty cool. I did some r-run and poked at the Redis data. TTL looks much better. 😄

Aside: Hadn't seen the redis MONITOR command before. That's pretty handy. (It's interesting to see a few anomalies in key names, but that's by-the-by...)

Tests are passing.

Changes are focused on Redis + PrevNext, so I don't think there's much else to test.

Let's merge it

[eileenmcnaughton]

thanks @totten - I actually have an action for parsing Redis monitor output - https://github.com/eileenmcnaughton/org.wikimedia.systemtools/blob/master/Civi/Api4/Redislog.php#L32 - it also has some parsing for the query log - https://wikitech.wikimedia.org/wiki/Fundraising/Internal-facing/CiviCRM#Performance_tracking - which is what I use for getting to the bottom of performance issues