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

Fix that :PRIMED keys are seemingly not removed #574

Merged
merged 4 commits into from
Feb 8, 2021

Conversation

mhenrixon
Copy link
Owner

No description provided.

@mhenrixon mhenrixon self-assigned this Feb 8, 2021
@mhenrixon mhenrixon mentioned this pull request Feb 8, 2021
@@ -52,11 +52,11 @@ def call
# @return [Array<String>] an array of orphaned digests
#
def orphans
conn.zrevrange(digests.key, 0, -1).each_with_object([]) do |digest, result|
conn.zrevrange(digests.key, 0, -1).each_with_object([]) do |digest, memo|
next if belongs_to_job?(digest)
Copy link
Contributor

@ArturT ArturT Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this line could prevent deleting PRIMED keys.

I tested below in rails console development. I have 2 left PRIMED keys

2.7.1 :024 > puts keys = REDIS_SIDEKIQ.keys('uniquejobs-v2:*')
uniquejobs-v2:44b417c0a889935af72fdec392b6924b:PRIMED
uniquejobs-v2:0e28561269199b1a7a6bee6e3dfac0a3:PRIMED

When I check orphans then it's an empty array. I guess it should return 2 digests for PRIMED keys so they could be deleted.

2.7.1 :027 > r=SidekiqUniqueJobs::Orphans::RubyReaper.new(REDIS_SIDEKIQ)
2.7.1 :028 > r.orphans
 => []

Another idea. This line belongs_to_job?(digest) might assume the PRIMED keys won't exist alone in Redis so it does not detect them. Maybe some level earlier there is a bug that leads to PRIMED keys existing alone in Redis.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we don't care about that here, we are only checking the digests. When the digest is no longer found in neither on queue, in scheduled, retry or process sets then be collect all those digests and delete their keys.

Unfortunately, I had a problem in my test setup that assumed the created_at date was on the item/job but it actually was on the payload and for whatever reason, the payload was a string which would prevent jobs from being reaped.

I also realized that I was keeping the primed, queued and info keys around forever for no good reason so I expire them regardless now

@ArturT could you test against this branch and see if the problem persists?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To elaborate a bit on the process:

  1. The lock is enqueued
  2. If we can steal (own) the queued token, we move it over to primed
  3. The primed token is popped and the lock is created

The reaper only considers the digest itself since that is the thing that follows the sidekiq job in { payload: { lock_digest: 'uniquejobs:sdasdadsasd' } } I only collect the ones for reaping that matches that.

In the end, all keys for said locks are cleared. Seems that despite almost 99% coverage I still have some testing scenarios to write. :)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the feedback and excellent error reporting @ArturT . It really helped me get to the bottom of this quickly. It seems I would also have to do some load tests. I was only able to find these problems during pretty high load.

Next up is more tests and some performance improvements (thanks to your feedback).

This prevents the reaper from crashing
There shouldn't be any need for these keys after a lock has been achieved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants