Closed gstokkink closed 7 months ago
@ixti something I ran into when trying out version 1.3.
Not sure how the tests are failing, they succeed locally for both .rspec
and .rspec.sidekiq-pro
🤔 Maybe because I use the BUNDLE_GEMS__CONTRIBSYS__COM
environment variable?
@gstokkink Sidekiq::Throttled.setup!
is deprecated and will be removed in v2. We don't call it and haven't run into issues. Do you still experience the issue if you remove the call to Sidekiq::Throttled.setup!
?
@gstokkink reading through your PR, do you need to keep .setup!
? I see that you're removing the deprecation warning, so I'm wondering if you're using .setup!
for more intricate setup than just reordering middlewares than what could be accomplished otherwise, like mentioned in this comment https://github.com/ixti/sidekiq-throttled/pull/167#issuecomment-1906739350
Last thing, your specs pass for me locally once I add this fix to spec_helper.rb
#181
I also noticed that .rspec-local
is not being respected and to update .rspec
itself
@mnovelo looks like I spoke too soon, I was accidentally still running the 1.0.0 release which did have this bug. In 1.3.0 it seems to not be an issue, because with the new implementation a job cannot block itself. Thus, if a job is orphaned and recovered, and it held a slot, it will immediately be allowed to run again, using that slot. No need to call finalize!
for the recovered job then. The only problem that I can foresee is that the slot will remain occupied if the job is never recovered, but that would mean SuperFetch is broken I guess. Or will it expire?
The reason setup!
was needed again is to register the orphan handler callback that calls finalize!
correctly, as I mentioned in the OP.
oh awesome! Thanks anyway for your help identifying that the Sidekiq Pro specs weren't working as expected.
Re: your question
The only problem that I can foresee is that the slot will remain occupied if the job is never recovered, but that would mean SuperFetch is broken I guess. Or will it expire?
SIdekiq-throttle's slots expire after 15 minutes by default.. Sidekiq-throttle calls .finalize!
in an ensure block, so if that block never gets called, the slot will be released in about 15 minutes, depending on the TTL set
Without this fix, previously throttled orphaned jobs would remain throttled after being recovered and placed back on the queue.
One downside is that this necessitates the usage of
setup!
once more, as the orphan recovery handler must be registered after SuperFetch has been initialized withconfig.super_Fetch!
.Also fixed the
bundle exec rake
script when running Sidekiq Pro tests with theBUNDLE_GEMS__CONTRIBSYS__COM
environment variable set.NOTE:
.rspec-local
does not work (at least with rspec 3.12), you have to override.rspec
instead.