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

Deprecate Sidekiq::Throttled.setup! #167

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Deprecate Sidekiq::Throttled.setup! #167

merged 1 commit into from
Nov 21, 2023

Conversation

ixti
Copy link
Owner

@ixti ixti commented Nov 21, 2023

No description provided.

* Rename `Sidekiq::Throttled::Middleware`
  to: `Sidekiq::Throttled::Middlewares::Server`
* `Sidekiq::Throttled.setup!` will behave same as before, but will
  print deprecation warning.

In rare case when middleware order matters, manupulate chains directly:

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

Signed-off-by: Alexey Zapparov <alexey@zapparov.com>
@ixti ixti merged commit bd63f83 into main Nov 21, 2023
4 checks passed
@ixti ixti deleted the refactor-middlewares branch November 21, 2023 06:18
@samrjenkins
Copy link

@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
Copy link
Owner Author

ixti commented Jan 23, 2024

@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
Copy link

Thank you for the clarification

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

Successfully merging this pull request may close these issues.

2 participants