-
Notifications
You must be signed in to change notification settings - Fork 75
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
Allow configuring whether throttled jobs are put back on the queue immediately or scheduled for the future #150
base: main
Are you sure you want to change the base?
Conversation
That is awesome! Thank you! I will take a look this weekend. |
Re: Style/RedundantCurrentDirectoryInPath I prefer consistency. It is pretty weird to me that rubocop's default for On a side note about this PR, I think I made a bad decision back when I started this gem on providing custom def self.included(base)
base.sidekiq_class_attribute :sidekiq_throttle_push_back # :enqueue | :schedule
end In this case we will not need to bother about "keeping track" of job class inheritance. |
0491614
to
a3503c1
Compare
Makes sense! I've updated the PR to disable the cop instead of applying it. |
Ah, interesting! I didn't know about Would you prefer |
a3503c1
to
b71ccdd
Compare
class MyJob
include Sidekiq::Job
include Sidekiq::Throttled::Job
sidekiq_throttled_push_back :enqueue
end Naturally, we should check if the value is Proc, and if so - we should call that proc to get the value, so that it will be possible to: class MyJob
include Sidekiq::Job
include Sidekiq::Throttled::Job
sidekiq_throttled_push_back ->(user_id, *) { user_id.odd? ? :enqueue : :schedule }
def perform(user_id, more, stuff)
# ...
end
end |
I have no strong feelings about Also, just realized, that it would be nice to also allow specifying which queue it should be requeued to. |
Hmm, I've been working on getting the def self.included(worker)
worker.send(:extend, ClassMethods)
worker.sidekiq_class_attribute :sidekiq_throttled_requeue_with # :enqueue | :schedule
end then the job class needs to use it like this: class MyThrottledJob
include Sidekiq::Job
include Sidekiq::Throttled::Job
self.sidekiq_throttled_requeue_with = :schedule
sidekiq_throttle foo: :bar i.e. it ends up as a class-level variable that needs to be assigned, rather than being something we can use like |
So maybe we invoke |
ec851bb
to
37c905a
Compare
b314a8d
to
122e6e1
Compare
Okay! I've made a number of changes. The basic usage should now look like this: sidekiq_throttle threshold: {limit: 123, period: 1.hour}, requeue: {to: :other_queue, with: :schedule} with support for Procs: class MyJob
include Sidekiq::Job
include Sidekiq::Throttled::Job
sidekiq_throttle threshold: {limit: 123, period: 1.hour},
requeue: {to: ->(user_id, *) { user_id.odd? ? :odd_queue, :even_queue },
with: ->(_user_id, more, *) { more > 25 ? :schedule : :enqueue }}
def perform(user_id, more, stuff)
# ...
end
end and the default configuration looks like this: Sidekiq::Throttled.configuration.default_requeue_options = {with: :schedule, to: :my_throttled_jobs_queue} Both |
Hey @ixti , |
Excited to land this PR both for the app I help maintain with @lavaturtle and another product I'm working on. Anything else we can do to help move this and v1 across the finish line? Happy to put some more of our engineering time in on our side if there are discrete tasks you can point us at. We're keen to invest in the gem & code quality. |
@lavaturtle @woodhull thanks for the amazing PR. I will try to go through it today-tomorrow. Sorry for being unresponsive (was really overwhelmed with some work lately - but slowly getting back at dedicating time to sidekiq-throttled). |
The only issue I'm scratching my head about right now is the Sidekiq-Pro support. Can't figure out how to make sure it works correctly. Thus thinking on skipping it's support for 1.0.0 release completely and circle back at that task after 1.0.0 release. |
@ixti I'm happy to help with the Sidekiq Pro support, regardless of whether it's for 1.0.0 or later! Let me know if there's a branch you'd like me to test and/or review |
Hey.... Just looping back to this. We're not using Sidekiq Pro so we aren't sure how to help move this forward on that front. That code is private for paid Sidekiq customers I think? |
@woodhull yeah, the Sidekiq Pro code is private for paid customers. @ixti I'm still happy to help with the Sidekiq Pro support, especially if it's blocking the 1.0.0 release. At the same time, I fully support releasing 1.0.0 without support for Sidekiq Pro's super_fetch. It's a configuration option that we're not using yet. We only use batching from Sidekiq Pro ourselves right now. |
Is this going to be released? I love this strategy, we're having a severly throttled queue, and it is degrading our other queues and jobs, would love this strategy! |
@JamesWatling yes. Working to merge this in the next couple of days. |
This PR will help to mitigate #86 as well |
I'm gonna refactor the way scheduling strategy is stored, and will incorporate this PR as part of v2.0.0 release which I will work on next weeks. Couple of heads up on the API I have in mind: sidekiq_throttle_push_back(
with: :enqueue, # Possible options: :enqueue, :requeue, and :schedule
to: nil, # Can specify queue to push back to, nil, String, or proc
in: 0 # only applicable for :schedule
) Under the hood, |
Hi there 👋 That's a pretty nice work going in there! 😊 I was wondering if it'd also make sense to have a Thanks! ❤️ |
Just checking in on this. Has it been abandoned? |
Nothing is abandoned. Just lacking of free time to work on this one :(( But it's on my radar to land first thing once I'll get to it. |
@ixti, let us know if we can help to move this forward! |
Looking forward to this PR as well, would solve the |
Oh and @ixti I'm happy to help with Sidekiq Pro testing if you require it 😄 |
This adds support for setting the
requeue_strategy
for a job, to specify what we should do with throttled jobs. The default is:enqueue
, which is the current behavior: re-add it to the end of the queue. The other option is:schedule
, which schedules the job for a time in the future when we think we'll have capacity to process it.It's also possible to set the
default_requeue_strategy
in the configuration, to set this behavior for all jobs that do not individually specify arequeue_strategy
.This may be relevant to Issue #36.
Unrelatedly, there's a commit in here that changes the format of some
require_relative
lines, to comply with the new Rubocop ruleStyle/RedundantCurrentDirectoryInPath
. I don't feel strongly about this commit; I only added it so that rubocop would pass.