Ensure batch delete removes expiring locks #724
Conversation
francesmcmullin
force-pushed
the
bug/del-from-expiring-zset
branch
3 times, most recently
from
6c70fa2
to
455a68c
Compare
francesmcmullin
force-pushed
the
bug/del-from-expiring-zset
branch
from
455a68c
to
f112f1d
Compare
|
|
||
| it "clears the lock" do | ||
| expect(redis { |conn| conn.zcard(SidekiqUniqueJobs::EXPIRING_DIGESTS) }).to eq 1 | ||
| sleep 2 |
There was a problem hiding this comment.
This spec passes for me locally, but I don't think a sleep this long is really appropriate even if it did work consistently. It's also not really the right place for a spec which is curious about the interaction between both the reaper and batch delete. It's here to demonstrate the issue, but I'd love your thoughts about a better location or tactic for testing this
There was a problem hiding this comment.
A sleep of two seconds would increase the test suite time with more than 50% (on my machine). Seems like something else is off here, I am without a computer until Monday but I'll check then.
|
Hey friends, what's the status on this PR? I hear this will help solve our rollout issues with using :until_expired at heavy scale (#637 (comment)). I'd be happy to run against this branch for a large-scale test if that'd be helpful, once it's ready and tests passing. |
Hi @mhenrixon thanks so much for the positive response to #721
Unfortunately I missed a spot! 馃槄 In truth, I think it's unnecessary to use batch delete when removing expiring locks - it would be enough to just remove the digests from the ZSET because all other elements are expired by redis, but since I did use batch delete - it's really important that batch delete removes from this new ZSET!
Previously, I only updated
lock.luato put these digests into a separate ZSET because that's what the locksmith calls, but when trying to verify this fix in tests, I foundlock.rbneeded to be updated as well, so that's here too.