Skip to content
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

Only throttle if parameter meets condition #30

Closed
dbenjamin57 opened this issue Mar 23, 2017 · 5 comments
Closed

Only throttle if parameter meets condition #30

dbenjamin57 opened this issue Mar 23, 2017 · 5 comments

Comments

@dbenjamin57
Copy link

Hi there, I was wondering if there was an option to throttle only if a param meets a certain condition.

Lets say for example I have Worker A:

class AWorker
include Sidekiq::Worker
def perform(some_param)
end
end

Now I only want to throttle the worker if some_param == "aaa" but not if some_param == anything else.
Is that a supported option the gem can handle? If so, how?

Cheers,
Dan

@ixti
Copy link
Owner

ixti commented Mar 23, 2017

it's not "supported" directly, but you can "workaround" that by:

class MyWorker
  include Sidekiq::Worker
  include Sidekiq::Throttled::Worker

  sidekiq_throttle(:concurrency => {
    # technically it's till throttling but using next to *more than enough* limit
    :limit => -> (user_id) { 1 == user_id ? 123_456_789 : 1_000 }
  })
end

We can improve API of Sidekiq::Throttled so that will not throttle if limit block returns nil. But I find that API pretty bad. IMHO in your case the best options would be:

class MyWorker
  include Sidekiq::Worker

  def perform(some_param)
    # ...
  end
end

class MyWorker::Throttled < MyWorker
  include Sidekiq::Throttled::Worker

  sidekiq_throttle :concurrency => { :limit => 1_000 }
end

@erated
Copy link

erated commented Mar 23, 2017

"We can improve API of Sidekiq::Throttled so that will not throttle if limit block returns nil. But I find that API pretty bad"

I actually like that approach. Why not go that route? The second option requires the users of the gem to create "ugly workarounds" just to support something that should be quite basic in terms of throttling.

not throttling if the deciding param is nil in my opinion is the cleanest solution.

@ixti
Copy link
Owner

ixti commented Mar 23, 2017

I did not said we won't go that way. I just said that I find it bad. I find dynamic throttling support we have bad as well, because it adds unneeded complexity. Any dynamically changed dependency in job's workflow makes it harder to debug. Proposed way of having separate class makes it simple and easy to figure out which part exactly fails.

@ixti
Copy link
Owner

ixti commented Mar 23, 2017

In any case, PRs are welcome. :D

ixti added a commit that referenced this issue Mar 27, 2017
@ixti
Copy link
Owner

ixti commented Mar 27, 2017

@erated, @dbenjamin57 see #31 (gonna merge and release as 0.7.1 this week)

@ixti ixti closed this as completed in #31 Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants