ixti / sidekiq-throttled

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

Sidekiq Pro paused queues incompatibility #87

Closed Loschcode closed 4 years ago

Loschcode commented 4 years ago

Paused queues on Sidekiq Pro don't work with this library. Basically, it'll have no effect when trying to pause a queue from the dashboard or programmatically when adding this line on the initializer.

Sidekiq::Throttled.setup!

I'm a little afraid the super fetch strategy simply doesn't work either since it's based on that. I've posted the issue on Sidekiq as well and that's when I discovered sidekiq-throttled was the problem https://github.com/mperham/sidekiq/issues/4684

Loschcode commented 4 years ago

My working solution regarding this problem was to monkey patch your library and remove #paused? which's in conflict with the pro version

module Sidekiq
  module Throttled
    module Patches
      module Queue
        remove_method :paused?
      end
    end
  end
end
ixti commented 4 years ago

Thank you for the report. Indeed, it was my mistake to mix paused queues logic with throttling... I plan to fix it in future though (already started my work on extracting throttling logic out).

Loschcode commented 4 years ago

Hey, despite closing it I've actually worked on a fork but couldn't make it work, do you think you could work on this in the near future? Or if you give me some advice on where to change the code I could do it myself actually.

I simply tried renaming #paused? but it's not enough, basically it breaks the throttling completely — but I can pause from Sidekiq Pro so there's that

ixti commented 4 years ago

@Loschcode I will make pausing related code optional as soon as I will have time for that and will extract it away at some point.

ixti commented 4 years ago

@Loschcode please try this branch: https://github.com/sensortower/sidekiq-throttled/tree/ixti/extract-pauser

ixti commented 4 years ago

branch name is a bit miseading :D

MutatedBread commented 9 months ago

Hi, may I ask if this received a fix already? We're digging into updating to latest version of sidekiq throttle from v0.18. Our setup includes sidekiq pro and used the old enhance queue tabs method to make the pause button in Queues tab to work.

mnovelo commented 9 months ago

We just updated to the latest sidekiq-throttle, v1.1.0, with the latest Sidekiq Pro, v7.2.0. We now only use the Sidekiq Pro ability to pause queues, so we were able to remove the deprecated enhanced queues tabs method, and did not have to install https://github.com/ixti/sidekiq-pauzer

It works just as well, though the UI isn't as nice imo

https://github.com/ixti/sidekiq-throttled/assets/5916364/1d2a7aa2-c43c-4bd7-b202-46375755316a

ixti commented 9 months ago

Hi, may I ask if this received a fix already? We're digging into updating to latest version of sidekiq throttle from v0.18. Our setup includes sidekiq pro and used the old enhance queue tabs method to make the pause button in Queues tab to work.

Enhanced Queues (and queue pausing feature in general) was not compatible with Sidekiq-Pro, thus it was extracted (and reworked) into a separate plugin: sidekiq-pauzer.

But sidekiq-throttled 1.1.0 is fully compaible with Sidekiq-Pro. And as Sidekiq-Pro has its own queue pause feature, it should be just fine. :D

ixti commented 9 months ago

It works just as well, though the UI isn't as nice imo

sidekiq-pauzer uses same queues view as sidekiq (I simply removed Sidekiq.pro? check), so it should be same as what you get in Sidekiq-Pro.