ixti / sidekiq-throttled

Concurrency and rate-limit throttling for Sidekiq
MIT License
698 stars 75 forks source link

Deprecate Sidekiq::Throttled.setup! #167

Closed ixti closed 9 months ago

samrjenkins commented 7 months ago

@ixti I am addressing the deprecation warning in our app and I wanted to get some clarification on why this method has be deprecated. Is the original issue of the ordering of middlewares no longer an issue?

I can see that by removing our Sidekiq::Throttled.setup! line, Sidekiq::Throttled::Middlewares::Server is no longer at the bottom of our server middleware chain. Should we expect that this will be a problem?

ixti commented 7 months ago

@samrjenkins setup! was doing more than just middlewares back in the days. When it was left with only adding middlewares I've decided to deprecate and eventually remove it in favour of using an explicit and more standard way to re-order middlewares.

There should be no difference where the middleware is called. But depending on other plugins, you might want to use a specific order of middlewares, and Sidekiq provides a simple way of doing that:

Sidekiq.configure do |config|
  config.server_middleware do |chain|
    chain.prepend(Sidekiq::Throttled::Middlewares::Server)
  end
end

Or the other way around:

Sidekiq.configure do |config|
  config.server_middleware do |chain|
    chain.add(Sidekiq::Throttled::Middlewares::Server)
  end
end

Or even add it before or after specific:

Sidekiq.configure do |config|
  config.server_middleware do |chain|
    chain.insert_before(OtherMiddleware, Sidekiq::Throttled::Middlewares::Server)
  end
end

But as I said, most likely you don't need that. The only purpose of this middleware is to remove the job from the concurrency lock. So if you don't experience any issues - you can use the defaults.

samrjenkins commented 7 months ago

Thank you for the clarification