Support Sidekiq Pro super_fetch #175
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## improve-composability-of-patches #175 +/- ##
====================================================================
- Coverage 98.94% 97.47% -1.47%
====================================================================
Files 18 19 +1
Lines 378 396 +18
Branches 52 54 +2
====================================================================
+ Hits 374 386 +12
- Misses 4 10 +6 ☔ View full report in Codecov by Sentry. |
| if work.respond_to?(:local_queue) | ||
| # if a SuperFetch UnitOfWork, SuperFetch will requeue it using lpush | ||
| work.requeue | ||
| else | ||
| # Fallback to BasicFetch behavior | ||
| redis { |conn| conn.lpush(work.queue, work.job) } | ||
| end |
There was a problem hiding this comment.
Been thinking about requeue logic in context of #150 and I think default logic should simply take unit_of_work's job and push it using Sidekiq::Client.push(message)
There was a problem hiding this comment.
Either way, that can be done separately (when I will have time to work on #150)
There was a problem hiding this comment.
I think it makes sense to change the behavior separately, keeping it in sync with the behavior for BasicFetch.
One caveat with using Sidekiq::Client.push(message) instead of conn.lpush is that it will overwrite the original enqueued_at https://github.com/sidekiq/sidekiq/blob/7df28434f03fa1111e9e2834271c020205369f94/lib/sidekiq/client.rb#L260
There was a problem hiding this comment.
The original enqueued_at value isn't really important to us though, so we'd be fine if it got replaced with each throttle
There was a problem hiding this comment.
The original
enqueued_atvalue isn't really important to us though, so we'd be fine if it got replaced with each throttle
It might be important for sidekiq-pro expiring jobs.
There was a problem hiding this comment.
Oh. I missed that it replaces enqueued_at. Let's keep it as is - as long as we can guarantee that it won't break a reliable (or whatever it is called) client. In either case, we need to call acknowledgement so that the job gets removed from the temporary queue.
There was a problem hiding this comment.
The requeue method of the SuperFetch UnitOfWork takes care of removing from the temporary queue, so I'll add that it does in a comment.
@freemanoid the expiring jobs middleware doesn't use enqueued_at
| if work.respond_to?(:local_queue) | ||
| # if a SuperFetch UnitOfWork, SuperFetch will requeue it using lpush | ||
| work.requeue | ||
| else | ||
| # Fallback to BasicFetch behavior | ||
| redis { |conn| conn.lpush(work.queue, work.job) } | ||
| end |
There was a problem hiding this comment.
The original
enqueued_atvalue isn't really important to us though, so we'd be fine if it got replaced with each throttle
It might be important for sidekiq-pro expiring jobs.
Co-authored-by: Alexey Zapparov <alexey@zapparov.com> Signed-off-by: Mauricio Novelo <mauricio.novelo@gmail.com>
mnovelo
force-pushed
the
sidekiq-pro-super-fetch
branch
from
d822386
to
b16e659
Compare
mnovelo
changed the base branch from
main
to
improve-composability-of-patches
|
o_O I swear I was not closing this PR |
|
Just merged the improve-composability-of-patches branch. |
|
@mnovelo please rebase this PR on main branch. |
|
GH does not allow me to even reopen this PR. O_o what the... |
|
Weird! I re-opened against the main branch |
Distilling the discussions in this issue and the work in this fork, this adds support for Sidekiq Pro's super_fetch for v1 of sidekiq-throttled and v7.x of Sidekiq Pro.