ixti / sidekiq-throttled

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

Support Sidekiq Pro super_fetch #178

Closed mnovelo closed 8 months ago

mnovelo commented 9 months ago

Distilling the discussions in this issue and the work in this fork, this adds support for Sidekiq Pro's super_fetch for v1 of sidekiq-throttled and v7.x of Sidekiq Pro.

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (bf88735) 98.94% compared to head (3c649ed) 98.95%.

:exclamation: Current head 3c649ed differs from pull request most recent head ea96cc3. Consider uploading reports for the commit ea96cc3 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #178 +/- ## ======================================= Coverage 98.94% 98.95% ======================================= Files 18 18 Lines 380 381 +1 Branches 53 53 ======================================= + Hits 376 377 +1 Misses 4 4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ixti commented 9 months ago

@mnovelo please add codecov path exclusion (.github/codecov.yml):

ignore:
  # Requires Sidekiq-Pro
  - lib/sidekiq/throttled/patches/super_fetch.rb
  - spec/lib/sidekiq/throttled/patches/super_fetch_spec.rb
ixti commented 9 months ago

After taking another look, I think there's a better way to exclude Sidekiq-Pro specs when it's not available. Let's add this change to spec_helper.rb:

diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index cb34b71..661327d 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -61,6 +61,9 @@ RSpec.configure do |config|
   # metadata: `fit`, `fdescribe` and `fcontext`, respectively.
   config.filter_run_when_matching :focus

+  # Skip tests that require Sidekiq-Pro
+  config.filter_run_excluding sidekiq_pro: true unless defined? Sidekiq::Pro
+
   # Allows RSpec to persist some state between runs in order to support
   # the `--only-failures` and `--next-failure` CLI options. We recommend
   # you configure your source control system to ignore this file.

Then spec/lib/sidekiq/throttled/patches/super_fetch_spec.rb will become:

RSpec.describe Sidekiq::Throttled::Patches::SuperFetch, :sidekiq_pro do
  let(:base_queue) { "default" }
  ...

So that spec suite will run only when Sidekiq-Pro is available, or when one runs it explicitly with:

bundle exec rspec --tag sidekiq_pro
ixti commented 8 months ago

@mnovelo I've added the above proposals (and a bit more) to a separate branch

ixti commented 8 months ago

Can you please check if the updated instructions make sense and specs pass for you https://github.com/ixti/sidekiq-throttled/tree/sidekiq-pro-super-fetch - and if so - I will merge it

mnovelo commented 8 months ago

Thanks! I was just wrapping them up and taking care of the Rubocop Metrics/CyclomaticComplexity: Cyclomatic complexity for active_queues is too high. warning. Will push in a moment

ixti commented 8 months ago

Thanks! I was just wrapping them up and taking care of the Rubocop Metrics/CyclomaticComplexity: Cyclomatic complexity for active_queues is too high. warning. Will push in a moment

Oh. hm... I wonder why Metrics/CyclomaticComplexity fails. It's not that complex LOL

mnovelo commented 8 months ago

Just added a change that removes some redundant safe operators since nil.to_a.to_h becomes {}. That took care of it 😊

ixti commented 8 months ago

I'm gonna squash merge this PR in a bit!

mnovelo commented 8 months ago

sorry, one moment. Specs aren't passing now

ixti commented 8 months ago

Oh.. Yeah. I'll wait.

ixti commented 8 months ago

sorry, one moment. Specs aren't passing now

Make sure to cp .rspec.sidekiq-pro .rspec-local, by default :sidekiq_pro tests are disabled, and .rspec.sidekiq-pro is a version of .rspec that does not filter them out.

mnovelo commented 8 months ago

Oh it's the 6.5 specs that are failing. I had it set to skip Sidekiq Pro < 7 since it uses a different config. For some reason, now they're running

mnovelo commented 8 months ago

Weird fluke in my local environment. Now everything is back and passing 😖 Anyway, ready to merge!

mnovelo commented 8 months ago

@ixti I applied this suggestion using the GitHub UI but it didn't get signed. Do you need me to fix that or does it not matter since you're gonna squash and merge?

ixti commented 8 months ago

@ixti I applied this suggestion using the GitHub UI but it didn't get signed. Do you need me to fix that or does it not matter since you're gonna squash and merge?

@mnovelo Yeah. Gonna squash merge it. So. don't worry about commits not being signed :D

ixti commented 8 months ago

@mnovelo Thank you very much! I'm gonna cut 1.3.0 release. and in 2.0.0 sidekiq 6.x will be fully dropped, so I am not worried about it :D

ixti commented 8 months ago

Released. Thank you very much to all who participated in this journey! :heart: