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

Move until_expired digests to separate zset #721

Merged

Conversation

francesmcmullin
Copy link
Contributor

@francesmcmullin francesmcmullin commented Jun 30, 2022

This is another potential solution to #684 . Although for our specific use case, we've moved away from sidekiq unique jobs for high traffic until_expired jobs, this may still be helpful for other users.

This is quite a large change, I appreciate I may need to improve my Lua, or make different refactorings.

In simple terms, the idea is to treat until_expired digests separately to the others. For all other digests, cleanup looks like this:

  1. The job is locked and the digest is saved. Possible when enqueueing, or at another time
  2. Something else happens, triggering lock cleanup - this can be the job finishing, or being picked off the queue for execution, all keys relating to the lock and the digest are deleted together.
  3. In rare cases, if something goes badly wrong above, the reaper will clean up the digest and related keys.

But for until_expired, cleanup looks like this:

  1. The job is locked and the digest is saved, usually when enqueueing
  2. The expiry time is reached, and all keys relating to the lock are deleted by redis's native expiry system, because they were all set with a TTL
  3. The digest, as a member of a ZSET, cannot have an expiry, so it must be cleaned up by the reaper

For this reason, if you use a lot of until_expired jobs, the reaper can become very slow and expensive. My proposed solution here is:

  • Do not save until_expired digests with the others, put them in their own separate ZSET, where the value (score) is the expiry time
  • When the reaper runs, delete by digest from the separate until_expired ZSET based on the score, skip the other (expensive) reaping checks.

@mhenrixon
Copy link
Owner

This is a great suggestion, can't believe I didn't think about this before. Would love to see a green test suite here :)

@francesmcmullin francesmcmullin force-pushed the optimise-reaping-expiry-locks branch 18 times, most recently from c34bf21 to b490edd Compare June 30, 2022 16:30
@francesmcmullin
Copy link
Contributor Author

Oh, I'm glad you like the idea! It's green now 🎉 🟢 Although I did add a few exceptions for reek/rubocop - hopefully these are OK

@mhenrixon
Copy link
Owner

mhenrixon commented Jul 1, 2022

I'll hold off on releasing this for a few days because I need to consider how this affects an upgrade. In your case (if you are still using until_expired) you'd end up with quite a lot of keys that the reaper must clean up.

I wonder what the damage would be for people upgrading and what version to create next.

@mhenrixon mhenrixon enabled auto-merge (squash) July 1, 2022 07:53
@mhenrixon mhenrixon merged commit bf573a4 into mhenrixon:main Jul 1, 2022
@francesmcmullin francesmcmullin deleted the optimise-reaping-expiry-locks branch July 1, 2022 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants