Allow configuring whether throttled jobs are put back on the queue immediately or scheduled for the future

[lavaturtle]

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 a requeue_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 rule Style/RedundantCurrentDirectoryInPath. I don't feel strongly about this commit; I only added it so that rubocop would pass.

[ixti]

That is awesome! Thank you! I will take a look this weekend.

[ixti]

Re: Style/RedundantCurrentDirectoryInPath

I prefer consistency. It is pretty weird to me that rubocop's default for rescue is to be explicit, while for require_relative it's implicit. Let's just disable this cop. I will find time to open a rubocop PR to make that configurable. But for now, I would like it to be simply disabled.

On a side note about this PR, I think I made a bad decision back when I started this gem on providing custom sidekiq_throttle singleton class message, probably we should utilize sidekiq_class_attribute helper instead:

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.

[lavaturtle]

Re: Style/RedundantCurrentDirectoryInPath

I prefer consistency. It is pretty weird to me that rubocop's default for rescue is to be explicit, while for require_relative it's implicit. Let's just disable this cop. I will find time to open a rubocop PR to make that configurable. But for now, I would like it to be simply disabled.

Makes sense! I've updated the PR to disable the cop instead of applying it.

[lavaturtle]

On a side note about this PR, I think I made a bad decision back when I started this gem on providing custom sidekiq_throttle singleton class message, probably we should utilize sidekiq_class_attribute helper instead:

Ah, interesting! I didn't know about sidekiq_class_attribute. So if we do that, then a job class can specify sidekiq_throttle_push_back in its sidekiq_options, and then we can read it later with get_sidekiq_options?

Would you prefer push_back as the setting name rather than requeue_strategy?

[ixti]

sidekiq_class_attribute :sidekiq_throttle_push_back will define a new singleton class method, so the usage will look like:

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

[ixti]

I have no strong feelings about push_back vs requeue_strategy. I just don't like over-use of strategy in names (easy to confuse with Throttled::Strategy). Probably simply: sidekiq_throttled_requeue_with?

Also, just realized, that it would be nice to also allow specifying which queue it should be requeued to.

[lavaturtle]

Hmm, I've been working on getting the sidekiq_class_attribute approach to work, and as far as I can tell from my testing, if we do this in Sidekiq::Throttled::Job:

      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 sidekiq_throttled_requeue_with :schedule.

[lavaturtle]

So maybe we invoke sidekiq_class_attribute for the inheritance benefits, but instead of users using sidekiq_throttled_requeue_with in the class directly, we keep the requeue_with option for sidekiq_throttle? That also avoids any weirdness around which order the two are called in.

[lavaturtle]

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 :with and :to arguments are optional. If :with is not specified, we'll use the :enqueue method. If :to is not specified, we'll use the queue the job was originally enqueued into.

[danpolyuha]

Hey @ixti ,
Thanks for your amazing work on this gem.
Sorry for asking here, just don't want to create new threads,
Do you have any estimate on release of 1.0.0 stable?

[woodhull]

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.

[ixti]

@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).

[ixti]

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.

[mnovelo]

@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

[woodhull]

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?

[mnovelo]

the Sidekiq Pro specs pass too 🎉

[anero]

Great, thanks for testing @mnovelo!

I was concerned that there may be a problem with how we requeue the job here https://github.com/ixti/sidekiq-throttled/pull/150/files#diff-c15f45918c6d7d825fbfa0856557360b907b492e44879ba9eb1e7f3bad9c42baR89 which is different from how it was previously done (https://github.com/ixti/sidekiq-throttled/pull/150/files#diff-ae016bb4af61725f74c58dac11f63bd38a08d6c5c5519e467902911a51077b0fL27), but I don't know enough about Sidekiq's API to say if it's an actual problem

[mnovelo]

oh, that is a problem actually @anero I missed that change. Sidekiq Pro's super_fetch does do important additional logic when requeuing. I can work on a PR to address this, but I would like it if we kept Throttled.requeue_throttled(work) or similar so that super_fetch can run its additional logic

[ixti]

IMO it's better to introduce #configure! on Sidekiq::Throttledto reconfigure if needed. As#resetwill only work duringconfigure` anyway (config gets frozen)

[anero]

I'm not sure I follow you. I understand that this will only work if used as we do here.
What would be the difference that you propose between Sidekiq::Throttled#configure! and Sidekiq::Throttled#configure? I thought a reset! method on Config would be cleaner to set all settings back to their default values. Alternatively I think something like the following would work:

Sidekiq::Throttled.configure do |config|
  config.cooldown_period    = 1.0
  config.cooldown_threshold = 100
  config.default_requeue_options = { with: :enqueue }
end

The problem IMO would be that you'd leak the default values out of Config. Even if declared as constants, I assumed it'd be more correct to keep the defaults encapsulated on the config class.

[ixti]

The problem is that config gets frozen after the #configure block. So you can't call Sidekiq::Throttled.config.reset!, thus with current API one will have to:

Sidekiq::Throttled.configure do |config|
  config.reset!

  ...
end

Versus:

Sidekiq::Throttled.configure! do |config|
  ...
end

[ixti]

After thinking a bit, I think config.reset! is actually cleaner approach :D

[ixti]

Let's move this into spec/support/sidekiq

[anero]

oh, that is a problem actually @anero I missed that change. Sidekiq Pro's super_fetch does do important additional logic when requeuing. I can work on a PR to address this, but I would like it if we kept Throttled.requeue_throttled(work) or similar so that super_fetch can run its additional logic

@mnovelo I wasn't sure if there's a way to reference the correct retriever (BasicFetch / SuperFetch) from Sidekiq::Throttled::Strategy#requeue_throttled.
I guess we could perform a similar verification as it's done elsewhere to decide whether Sidekiq Pro is available or not, and requeue using the correct mechanism. Do you've any thoughts on this @ixti ?

[mnovelo]

@anero oh, the class of the work variable will be Sidekiq::Pro::SuperFetch::UnitOfWork, so that's one way to distinguish whether the super_fetch strategy should be used. That was already being assumed in the previous implementation because it just delegated the requeuing logic to Sidekiq::Pro::SuperFetch::UnitOfWork#requeue so something like

case work
when Sidekiq::Pro::SuperFetch::UnitOfWork
# super_fetch stuff
else
# basic_fetch stuff
end

could work. Though I haven't dug deep into this yet

[ixti]

I think, the most clean solution will be to patch UnitOfWork similar way we patch BasicFetch and SuperFetch. What @mnovelo proposed will work even better, though. :D

[mnovelo]

ok, I'll work on a PR into this one then @anero @ixti !

[mnovelo]

@anero @ixti here's my proposal controlshift#2

If this looks good, I can write specs for it too

[anero]

Thanks for the PR @mnovelo, sorry I merged before seeing your comment on the specs. Want to add those separately? (I'd do it myself but I don't have Sidekiq Pro to be able to run them)

[anero]

@mnovelo I fixed the failing specs. Would you mind adding test coverage for the new code on Sidekiq::Throttled::Strategy to raise code coverage and get the build passing?

[mnovelo]

@anero Specs are ready for your review! controlshift#3

[mnovelo]

This is looking good! However, I want to highlight something important about the new scheduling strategy for future reference. It is not fully compatible with Sidekiq::Batch because when a job gets rescheduled using this strategy, it is assigned a new job ID. If a job within a batch gets throttled with the new scheduling strategy, the original job will be marked as complete. This could lead the batch to assume that the job has been completed as well. I don't think the new job retains the batch ID, and I didn't make any effort to ensure this or write any specs to test this. In my opinion, there isn't a strong use case for using both the new scheduling strategy and Sidekiq::Batch together. However, I'm happy to help revisit that in the future should the need arise.

[anero]

@ixti do you think you'll have time to review and merge (assuming you're OK with @mnovelo comment)?

[ixti]

Yeah. I will review, apply changes (if will have any) and merge this weekend.

[ixti]

Most likely merge as is - don't want to hold up this any longer ))

[ixti]

@freemanoid if you can take a look as well - that would be splendid!

[ixti]

I'm going to release this as v2.0.0.pre1 and open a PR with the refactoring I have in mind later.

I will release this change and the cooldown defaults changes as v1.5.0 instead. Refactoring I have in mind is to make sure requeue throttled would work fine with Job-Iteration alike API and be a bit more isolated from the throttling strategies. Thus that change (when I will have time for it) will deserve v2.0.0 :D

[ixti]

Release: #197