From 03e0cd21b2bab188421b8d8032fb14fc35a6d295 Mon Sep 17 00:00:00 2001 From: Grey Moore Date: Tue, 27 Jun 2023 16:57:53 -0400 Subject: [PATCH 01/62] Job can have a requeue_strategy, which is always :enqueue --- lib/sidekiq/throttled.rb | 13 ++++++++ lib/sidekiq/throttled/patches/basic_fetch.rb | 13 +------- lib/sidekiq/throttled/strategy.rb | 34 ++++++++++++++++++-- 3 files changed, 45 insertions(+), 15 deletions(-) diff --git a/lib/sidekiq/throttled.rb b/lib/sidekiq/throttled.rb index 8a549958..dd47902c 100644 --- a/lib/sidekiq/throttled.rb +++ b/lib/sidekiq/throttled.rb @@ -70,6 +70,19 @@ def throttled?(message) rescue false end + + # Return throttled job to be executed later, delegating the details of how to do that + # to the Strategy for that job. + # + # @return [void] + def requeue_throttled(work) + message = JSON.parse(work.job) + job_class = message.fetch("wrapped") { message.fetch("class") { return false } } + + Registry.get job_class do |strategy| + strategy.requeue_throttled(work) + end + end end end diff --git a/lib/sidekiq/throttled/patches/basic_fetch.rb b/lib/sidekiq/throttled/patches/basic_fetch.rb index c421520d..503092bb 100644 --- a/lib/sidekiq/throttled/patches/basic_fetch.rb +++ b/lib/sidekiq/throttled/patches/basic_fetch.rb @@ -20,7 +20,7 @@ def retrieve_work work = super if work && Throttled.throttled?(work.job) - requeue_throttled(work) + Throttled.requeue_throttled(work) return nil end @@ -29,17 +29,6 @@ def retrieve_work private - # Pushes job back to the head of the queue, so that job won't be tried - # immediately after it was requeued (in most cases). - # - # @note This is triggered when job is throttled. So it is same operation - # Sidekiq performs upon `Sidekiq::Worker.perform_async` call. - # - # @return [void] - def requeue_throttled(work) - redis { |conn| conn.lpush(work.queue, work.job) } - end - # Returns list of queues to try to fetch jobs from. # # @note It may return an empty array. diff --git a/lib/sidekiq/throttled/strategy.rb b/lib/sidekiq/throttled/strategy.rb index 6a1b1dea..280fd586 100644 --- a/lib/sidekiq/throttled/strategy.rb +++ b/lib/sidekiq/throttled/strategy.rb @@ -24,6 +24,12 @@ class Strategy # @return [Proc, nil] attr_reader :observer + # @!attribute [r] requeue_strategy + # @return [String] + attr_reader :requeue_strategy + + REQUEUE_STRATEGIES = [:enqueue].freeze + # @param [#to_s] name # @param [Hash] concurrency Concurrency options. # See keyword args of {Strategy::Concurrency#initialize} for details. @@ -31,8 +37,10 @@ class Strategy # See keyword args of {Strategy::Threshold#initialize} for details. # @param [#call] key_suffix Dynamic key suffix generator. # @param [#call] observer Process called after throttled. - def initialize(name, concurrency: nil, threshold: nil, key_suffix: nil, observer: nil) # rubocop:disable Metrics/MethodLength + # @param [#to_s] requeue_strategy What to do with jobs that are throttled + def initialize(name, concurrency: nil, threshold: nil, key_suffix: nil, observer: nil, requeue_strategy: :enqueue) # rubocop:disable Metrics/MethodLength, Metrics/ParameterLists @observer = observer + @requeue_strategy = requeue_strategy @concurrency = StrategyCollection.new(concurrency, strategy: Concurrency, @@ -44,9 +52,13 @@ def initialize(name, concurrency: nil, threshold: nil, key_suffix: nil, observer name: name, key_suffix: key_suffix) - return if @concurrency.any? || @threshold.any? + unless @concurrency.any? || @threshold.any? + raise ArgumentError, "Neither :concurrency nor :threshold given" + end - raise ArgumentError, "Neither :concurrency nor :threshold given" + unless REQUEUE_STRATEGIES.include?(@requeue_strategy) + raise ArgumentError, "#{requeue_strategy} is not a valid :requeue_strategy" + end end # @return [Boolean] whenever strategy has dynamic config @@ -74,6 +86,22 @@ def throttled?(jid, *job_args) false end + # Pushes job back to the head of the queue, so that job won't be tried + # immediately after it was requeued (in most cases). + # + # @note This is triggered when job is throttled. So it is same operation + # Sidekiq performs upon `Sidekiq::Worker.perform_async` call. + # + # @return [void] + def requeue_throttled(work) + case requeue_strategy + when :enqueue + Sidekiq.redis { |conn| conn.lpush(work.queue, work.job) } + else + raise "unrecognized requeue_strategy #{requeue_strategy}" + end + end + # Marks job as being processed. # @return [void] def finalize!(jid, *job_args) From b809c2faebc5eb5474af458b8725b041e4ba4d50 Mon Sep 17 00:00:00 2001 From: Grey Moore Date: Wed, 28 Jun 2023 16:01:27 -0400 Subject: [PATCH 02/62] Add :schedule option for requeue_strategy --- lib/sidekiq/throttled/strategy.rb | 36 +++-- lib/sidekiq/throttled/strategy/concurrency.rb | 10 ++ lib/sidekiq/throttled/strategy/threshold.rb | 19 +++ lib/sidekiq/throttled/strategy_collection.rb | 5 + .../throttled/strategy/concurrency_spec.rb | 56 ++++++++ .../throttled/strategy/threshold_spec.rb | 56 ++++++++ spec/lib/sidekiq/throttled/strategy_spec.rb | 133 ++++++++++++++++++ 7 files changed, 303 insertions(+), 12 deletions(-) diff --git a/lib/sidekiq/throttled/strategy.rb b/lib/sidekiq/throttled/strategy.rb index 280fd586..64d9de46 100644 --- a/lib/sidekiq/throttled/strategy.rb +++ b/lib/sidekiq/throttled/strategy.rb @@ -28,7 +28,7 @@ class Strategy # @return [String] attr_reader :requeue_strategy - REQUEUE_STRATEGIES = [:enqueue].freeze + REQUEUE_STRATEGIES = %i[enqueue schedule].freeze # @param [#to_s] name # @param [Hash] concurrency Concurrency options. @@ -52,13 +52,11 @@ def initialize(name, concurrency: nil, threshold: nil, key_suffix: nil, observer name: name, key_suffix: key_suffix) - unless @concurrency.any? || @threshold.any? - raise ArgumentError, "Neither :concurrency nor :threshold given" - end + raise ArgumentError, "Neither :concurrency nor :threshold given" unless @concurrency.any? || @threshold.any? - unless REQUEUE_STRATEGIES.include?(@requeue_strategy) - raise ArgumentError, "#{requeue_strategy} is not a valid :requeue_strategy" - end + return if REQUEUE_STRATEGIES.include?(@requeue_strategy) + + raise ArgumentError, "#{requeue_strategy} is not a valid :requeue_strategy" end # @return [Boolean] whenever strategy has dynamic config @@ -86,17 +84,18 @@ def throttled?(jid, *job_args) false end - # Pushes job back to the head of the queue, so that job won't be tried - # immediately after it was requeued (in most cases). - # - # @note This is triggered when job is throttled. So it is same operation - # Sidekiq performs upon `Sidekiq::Worker.perform_async` call. + # Return throttled job to be executed later. Implementation depends on the requeue_strategy. # # @return [void] def requeue_throttled(work) case requeue_strategy when :enqueue + # Push the job back to the head of the queue. + # This is the same operation Sidekiq performs upon `Sidekiq::Worker.perform_async` call. Sidekiq.redis { |conn| conn.lpush(work.queue, work.job) } + when :schedule + # Find out when we will next be able to execute this job, and reschedule for then. + reschedule_throttled(work) else raise "unrecognized requeue_strategy #{requeue_strategy}" end @@ -114,6 +113,19 @@ def reset! @concurrency&.reset! @threshold&.reset! end + + private + + def reschedule_throttled(work) + message = JSON.parse(work.job) + jid = message.fetch("jid") { return false } + job_class = message.fetch("wrapped") { message.fetch("class") { return false } } + job_args = message["args"] + + retry_in = [@concurrency&.retry_in(jid, *job_args), @threshold&.retry_in(*job_args)].compact.max + + Sidekiq::Client.enqueue_to_in(work.queue, retry_in, Object.const_get(job_class), *job_args) + end end end end diff --git a/lib/sidekiq/throttled/strategy/concurrency.rb b/lib/sidekiq/throttled/strategy/concurrency.rb index 9d9663c7..2f4e9376 100644 --- a/lib/sidekiq/throttled/strategy/concurrency.rb +++ b/lib/sidekiq/throttled/strategy/concurrency.rb @@ -52,6 +52,16 @@ def throttled?(jid, *job_args) Sidekiq.redis { |redis| 1 == SCRIPT.call(redis, keys: keys, argv: argv) } end + # @return [Float] How long, in seconds, before we'll next be able to take on jobs + def retry_in(_jid, *job_args) + job_limit = limit(job_args) + return 0.0 if !job_limit || count(*job_args) < job_limit + + oldest_jid_with_score = Sidekiq.redis { |redis| redis.zrange(key(job_args), 0, 0, withscores: true) }.first + expiry_time = oldest_jid_with_score.last.to_f + expiry_time - Time.now.to_f + end + # @return [Integer] Current count of jobs def count(*job_args) Sidekiq.redis { |conn| conn.zcard(key(job_args)) }.to_i diff --git a/lib/sidekiq/throttled/strategy/threshold.rb b/lib/sidekiq/throttled/strategy/threshold.rb index 11383d36..095f37e0 100644 --- a/lib/sidekiq/throttled/strategy/threshold.rb +++ b/lib/sidekiq/throttled/strategy/threshold.rb @@ -69,6 +69,25 @@ def throttled?(*job_args) Sidekiq.redis { |redis| 1 == SCRIPT.call(redis, keys: keys, argv: argv) } end + # @return [Float] How long, in seconds, before we'll next be able to take on jobs + def retry_in(*job_args) + job_limit = limit(job_args) + return 0.0 if !job_limit || count(*job_args) < job_limit + + job_period = period(job_args) + job_key = key(job_args) + time_since_oldest = Time.now.to_f - Sidekiq.redis { |redis| redis.lindex(job_key, -1) }.to_f + if time_since_oldest > job_period + # The oldest job on our list is from more than the throttling period ago, + # which means we have not hit the limit this period. + 0.0 + else + # If we can only have X jobs every Y minutes, then wait until Y minutes have elapsed + # since the oldest job on our list. + job_period - time_since_oldest + end + end + # @return [Integer] Current count of jobs def count(*job_args) Sidekiq.redis { |conn| conn.llen(key(job_args)) }.to_i diff --git a/lib/sidekiq/throttled/strategy_collection.rb b/lib/sidekiq/throttled/strategy_collection.rb index ce9d7e34..867c9c98 100644 --- a/lib/sidekiq/throttled/strategy_collection.rb +++ b/lib/sidekiq/throttled/strategy_collection.rb @@ -41,6 +41,11 @@ def throttled?(*args) any? { |s| s.throttled?(*args) } end + # @return [Float] How long, in seconds, before we'll next be able to take on jobs + def retry_in(*args) + max { |s| s.retry_in(*args) } + end + # Marks job as being processed. # @return [void] def finalize!(*args) diff --git a/spec/lib/sidekiq/throttled/strategy/concurrency_spec.rb b/spec/lib/sidekiq/throttled/strategy/concurrency_spec.rb index 5995e1c0..c0968bc0 100644 --- a/spec/lib/sidekiq/throttled/strategy/concurrency_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy/concurrency_spec.rb @@ -40,6 +40,62 @@ end end + describe "#retry_in" do + context "when limit is exceeded with all jobs starting just now" do + before { 5.times { strategy.throttled? jid } } + + it "tells us to wait roughly one ttl" do + expect(subject.retry_in(jid)).to be_within(0.1).of(900) + end + end + + context "when limit exceeded, with first job starting 800 seconds ago" do + before do + Timecop.travel(Time.now - 800) do + strategy.throttled? jid + end + 4.times { strategy.throttled? jid } + end + + it "tells us to wait 100 seconds" do + expect(subject.retry_in(jid)).to be_within(0.1).of(100) + end + end + + context "when limit not exceeded, because the oldest job was more than the ttl ago" do + before do + Timecop.travel(Time.now - 1000) do + strategy.throttled? jid + end + 4.times { strategy.throttled? jid } + end + + it "tells us we do not need to wait" do + expect(subject.retry_in(jid)).to eq 0 + end + end + + context "when limit not exceeded, because there are fewer jobs than the limit" do + before do + 4.times { strategy.throttled? jid } + end + + it "tells us we do not need to wait" do + expect(subject.retry_in(jid)).to eq 0 + end + end + + context "when dynamic limit returns nil" do + let(:strategy) { described_class.new :test, limit: proc { |*| } } + + before { 5.times { strategy.throttled? jid } } + + it "tells us we do not need to wait" do + expect(subject.retry_in(jid)).to eq 0 + end + end + end + describe "#count" do subject { strategy.count } diff --git a/spec/lib/sidekiq/throttled/strategy/threshold_spec.rb b/spec/lib/sidekiq/throttled/strategy/threshold_spec.rb index 3ab6d126..ab692be5 100644 --- a/spec/lib/sidekiq/throttled/strategy/threshold_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy/threshold_spec.rb @@ -40,6 +40,62 @@ end end + describe "#retry_in" do + context "when limit exceeded with all jobs happening just now" do + before { 5.times { strategy.throttled? } } + + it "tells us to wait roughly one period" do + expect(subject.retry_in).to be_within(0.1).of(10) + end + end + + context "when limit exceeded, with first job happening 8 seconds ago" do + before do + Timecop.travel(Time.now - 8) do + strategy.throttled? + end + 4.times { strategy.throttled? } + end + + it "tells us to wait 2 seconds" do + expect(subject.retry_in).to be_within(0.1).of(2) + end + end + + context "when limit not exceeded, because the oldest job was more than a period ago" do + before do + Timecop.travel(Time.now - 12) do + strategy.throttled? + end + 4.times { strategy.throttled? } + end + + it "tells us we do not need to wait" do + expect(subject.retry_in).to eq 0 + end + end + + context "when limit not exceeded, because there are fewer jobs than the limit" do + before do + 4.times { strategy.throttled? } + end + + it "tells us we do not need to wait" do + expect(subject.retry_in).to eq 0 + end + end + + context "when there is no limit" do + subject(:strategy) { described_class.new :test, limit: -> {}, period: 10 } + + before { 5.times { strategy.throttled? } } + + it "tells us we do not need to wait" do + expect(subject.retry_in).to eq 0 + end + end + end + describe "#count" do subject { strategy.count } diff --git a/spec/lib/sidekiq/throttled/strategy_spec.rb b/spec/lib/sidekiq/throttled/strategy_spec.rb index 230150e0..b066313b 100644 --- a/spec/lib/sidekiq/throttled/strategy_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy_spec.rb @@ -1,5 +1,12 @@ # frozen_string_literal: true +class ThrottledTestJob + include Sidekiq::Job + include Sidekiq::Throttled::Job + + def perform(*); end +end + RSpec.describe Sidekiq::Throttled::Strategy do subject(:strategy) { described_class.new(:foo, **options) } @@ -199,6 +206,132 @@ end end + describe "#requeue_throttled" do + let(:sidekiq_config) do + if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") + Sidekiq::DEFAULTS + else + Sidekiq::Config.new(queues: %w[default]).default_capsule + end + end + + let!(:work) do + # Sidekiq is FIFO queue, with head on right side of the list, + # meaning jobs below will be stored in 3, 2, 1 order. + ThrottledTestJob.perform_bulk([[1], [2], [3]]) + + # Pop the work off the queue + job = Sidekiq.redis do |conn| + conn.rpop("queue:default") + end + Sidekiq::BasicFetch::UnitOfWork.new("queue:default", job, sidekiq_config) + end + + context "with requeue_strategy = :enqueue" do + def enqueued_jobs(queue) + Sidekiq.redis do |conn| + conn.lrange("queue:#{queue}", 0, -1).map do |job| + JSON.parse(job).then do |payload| + [payload["class"], payload["args"]] + end + end + end + end + let(:options) { threshold.merge(requeue_strategy: :enqueue) } + + it "puts the job back on the queue" do + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + + # Requeue the work + subject.requeue_throttled(work) + + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], + ["ThrottledTestJob", [2]]]) + end + end + + context "with requeue_strategy = :schedule" do + let(:options) { basic_options.merge(requeue_strategy: :schedule) } + + context "when threshold constraints given" do + let(:basic_options) { threshold } + + before do + allow(subject.threshold).to receive(:retry_in).and_return(300.0) + end + + it "reschedules for when the threshold strategy says to" do + # Requeue the work, see that it ends up in 'schedule' + expect { subject.requeue_throttled(work) }.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = Sidekiq.redis do |conn| + # Depending on whether we have redis-client (for Sidekiq 7) or redis-rb (for older Sidekiq), + # zscan takes different arguments + if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") + conn.zscan("schedule", 0).last.first + else + conn.zscan("schedule").first + end + end + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") + expect(score.to_f).to be_within(5.0).of(Time.now.to_f + 300.0) + end + end + + context "when concurrency constraints given" do + let(:basic_options) { concurrency } + + before do + allow(subject.concurrency).to receive(:retry_in).and_return(300.0) + end + + it "reschedules for when the concurrency strategy says to" do + # Requeue the work, see that it ends up in 'schedule' + expect { subject.requeue_throttled(work) }.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = Sidekiq.redis do |conn| + # Depending on whether we have redis-client (for Sidekiq 7) or redis-rb (for older Sidekiq), + # zscan takes different arguments + if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") + conn.zscan("schedule", 0).last.first + else + conn.zscan("schedule").first + end + end + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") + expect(score.to_f).to be_within(5.0).of(Time.now.to_f + 300.0) + end + end + + context "when threshold and concurrency constraints given" do + let(:basic_options) { threshold.merge concurrency } + + before do + allow(subject.concurrency).to receive(:retry_in).and_return(300.0) + allow(subject.threshold).to receive(:retry_in).and_return(500.0) + end + + it "reschedules for the later of what the two say" do + # Requeue the work, see that it ends up in 'schedule' + expect { subject.requeue_throttled(work) }.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = Sidekiq.redis do |conn| + # Depending on whether we have redis-client (for Sidekiq 7) or redis-rb (for older Sidekiq), + # zscan takes different arguments + if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") + conn.zscan("schedule", 0).last.first + else + conn.zscan("schedule").first + end + end + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") + expect(score.to_f).to be_within(5.0).of(Time.now.to_f + 500.0) + end + end + end + end + describe "#reset!" do context "when only concurrency constraint given" do let(:options) { concurrency } From 68710ee7ec98ce123af923230da90f053fc97a53 Mon Sep 17 00:00:00 2001 From: Grey Moore Date: Thu, 29 Jun 2023 08:54:46 -0400 Subject: [PATCH 03/62] allow setting default_requeue_strategy on configuration --- lib/sidekiq/throttled/configuration.rb | 11 +++++++ lib/sidekiq/throttled/strategy.rb | 4 +-- spec/lib/sidekiq/throttled/strategy_spec.rb | 36 +++++++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/lib/sidekiq/throttled/configuration.rb b/lib/sidekiq/throttled/configuration.rb index 70c69679..1bee4427 100644 --- a/lib/sidekiq/throttled/configuration.rb +++ b/lib/sidekiq/throttled/configuration.rb @@ -4,6 +4,8 @@ module Sidekiq module Throttled # Configuration holder. class Configuration + attr_reader :default_requeue_strategy + # Class constructor. def initialize reset! @@ -14,6 +16,7 @@ def initialize # @return [self] def reset! @inherit_strategies = false + @default_requeue_strategy = :enqueue self end @@ -45,6 +48,14 @@ def inherit_strategies=(value) def inherit_strategies? @inherit_strategies end + + # Specifies how we should return throttled jobs to the queue so they can be executed later. + # Options are `:enqueue` (put them on the end of the queue) and `:schedule` (schedule for later). + # Default: `:enqueue` + # + def default_requeue_strategy=(value) + @default_requeue_strategy = value.intern + end end end end diff --git a/lib/sidekiq/throttled/strategy.rb b/lib/sidekiq/throttled/strategy.rb index 64d9de46..432229cd 100644 --- a/lib/sidekiq/throttled/strategy.rb +++ b/lib/sidekiq/throttled/strategy.rb @@ -38,9 +38,9 @@ class Strategy # @param [#call] key_suffix Dynamic key suffix generator. # @param [#call] observer Process called after throttled. # @param [#to_s] requeue_strategy What to do with jobs that are throttled - def initialize(name, concurrency: nil, threshold: nil, key_suffix: nil, observer: nil, requeue_strategy: :enqueue) # rubocop:disable Metrics/MethodLength, Metrics/ParameterLists + def initialize(name, concurrency: nil, threshold: nil, key_suffix: nil, observer: nil, requeue_strategy: nil) # rubocop:disable Metrics/MethodLength, Metrics/ParameterLists @observer = observer - @requeue_strategy = requeue_strategy + @requeue_strategy = requeue_strategy || Throttled.configuration.default_requeue_strategy @concurrency = StrategyCollection.new(concurrency, strategy: Concurrency, diff --git a/spec/lib/sidekiq/throttled/strategy_spec.rb b/spec/lib/sidekiq/throttled/strategy_spec.rb index b066313b..1e7b5a1e 100644 --- a/spec/lib/sidekiq/throttled/strategy_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy_spec.rb @@ -45,6 +45,42 @@ def perform(*); end key_suffix: key_suffix }) end + + it "uses :enqueue requeue_strategy by default" do + key_suffix = ->(_) {} + + instance = described_class.new(:foo, threshold: { limit: 123, period: 456, key_suffix: key_suffix }) + expect(instance.requeue_strategy).to eq :enqueue + end + + it "uses specified requeue_strategy" do + key_suffix = ->(_) {} + + instance = described_class.new(:foo, threshold: { limit: 123, period: 456, key_suffix: key_suffix }, + requeue_strategy: :schedule) + expect(instance.requeue_strategy).to eq :schedule + end + + context "when a default_requeue_strategy is set" do + before { Sidekiq::Throttled.configuration.default_requeue_strategy = :schedule } + + after { Sidekiq::Throttled.configuration.reset! } + + it "uses the default when not overridden" do + key_suffix = ->(_) {} + + instance = described_class.new(:foo, threshold: { limit: 123, period: 456, key_suffix: key_suffix }) + expect(instance.requeue_strategy).to eq :schedule + end + + it "allows overriding the default" do + key_suffix = ->(_) {} + + instance = described_class.new(:foo, threshold: { limit: 123, period: 456, key_suffix: key_suffix }, + requeue_strategy: :enqueue) + expect(instance.requeue_strategy).to eq :enqueue + end + end end describe "#throttled?" do From 39f255cdebfef40defef64c271adb592b0e9ec9b Mon Sep 17 00:00:00 2001 From: Grey Moore Date: Thu, 29 Jun 2023 09:03:30 -0400 Subject: [PATCH 04/62] add some random jitter --- lib/sidekiq/throttled/strategy.rb | 19 ++++++++++++++++--- spec/lib/sidekiq/throttled/strategy_spec.rb | 12 ++++++------ 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/lib/sidekiq/throttled/strategy.rb b/lib/sidekiq/throttled/strategy.rb index 432229cd..04ce33b8 100644 --- a/lib/sidekiq/throttled/strategy.rb +++ b/lib/sidekiq/throttled/strategy.rb @@ -118,13 +118,26 @@ def reset! def reschedule_throttled(work) message = JSON.parse(work.job) - jid = message.fetch("jid") { return false } job_class = message.fetch("wrapped") { message.fetch("class") { return false } } job_args = message["args"] - retry_in = [@concurrency&.retry_in(jid, *job_args), @threshold&.retry_in(*job_args)].compact.max + Sidekiq::Client.enqueue_to_in(work.queue, retry_in(work), Object.const_get(job_class), *job_args) + end + + def retry_in(work) + message = JSON.parse(work.job) + jid = message.fetch("jid") { return false } + job_args = message["args"] + + # Ask both concurrency and threshold, if relevant, how long minimum until we can retry. + # If we get two answers, take the longer one. + interval = [@concurrency&.retry_in(jid, *job_args), @threshold&.retry_in(*job_args)].compact.max + + # Add a random amount of jitter, proportional to the length of the minimum retry time. + # This helps spread out jobs more evenly and avoid clumps of jobs on the queue. + interval += rand(interval / 5) if interval > 10 - Sidekiq::Client.enqueue_to_in(work.queue, retry_in, Object.const_get(job_class), *job_args) + interval end end end diff --git a/spec/lib/sidekiq/throttled/strategy_spec.rb b/spec/lib/sidekiq/throttled/strategy_spec.rb index 1e7b5a1e..3dd8d91e 100644 --- a/spec/lib/sidekiq/throttled/strategy_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy_spec.rb @@ -297,7 +297,7 @@ def enqueued_jobs(queue) allow(subject.threshold).to receive(:retry_in).and_return(300.0) end - it "reschedules for when the threshold strategy says to" do + it "reschedules for when the threshold strategy says to, plus some jitter" do # Requeue the work, see that it ends up in 'schedule' expect { subject.requeue_throttled(work) }.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) @@ -311,7 +311,7 @@ def enqueued_jobs(queue) end end expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") - expect(score.to_f).to be_within(5.0).of(Time.now.to_f + 300.0) + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) end end @@ -322,7 +322,7 @@ def enqueued_jobs(queue) allow(subject.concurrency).to receive(:retry_in).and_return(300.0) end - it "reschedules for when the concurrency strategy says to" do + it "reschedules for when the concurrency strategy says to, plus some jitter" do # Requeue the work, see that it ends up in 'schedule' expect { subject.requeue_throttled(work) }.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) @@ -336,7 +336,7 @@ def enqueued_jobs(queue) end end expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") - expect(score.to_f).to be_within(5.0).of(Time.now.to_f + 300.0) + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) end end @@ -348,7 +348,7 @@ def enqueued_jobs(queue) allow(subject.threshold).to receive(:retry_in).and_return(500.0) end - it "reschedules for the later of what the two say" do + it "reschedules for the later of what the two say, plus some jitter" do # Requeue the work, see that it ends up in 'schedule' expect { subject.requeue_throttled(work) }.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) @@ -362,7 +362,7 @@ def enqueued_jobs(queue) end end expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") - expect(score.to_f).to be_within(5.0).of(Time.now.to_f + 500.0) + expect(score.to_f).to be_within(51.0).of(Time.now.to_f + 550.0) end end end From b71ccdd5a644c0fa28f3ba0fe5b0be560bf53286 Mon Sep 17 00:00:00 2001 From: Grey Moore Date: Thu, 29 Jun 2023 11:14:09 -0400 Subject: [PATCH 05/62] disable rubocop Style/RedundantCurrentDirectoryInPath --- rubocop/style.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/rubocop/style.yml b/rubocop/style.yml index 47b505f0..18f14c29 100644 --- a/rubocop/style.yml +++ b/rubocop/style.yml @@ -46,6 +46,9 @@ Style/OptionalBooleanParameter: Style/RedundantAssignment: Enabled: true +Style/RedundantCurrentDirectoryInPath: + Enabled: false + Style/RedundantFetchBlock: Enabled: true From 92b8d3d9ea57f3f732e15c27732502011c7f7b8c Mon Sep 17 00:00:00 2001 From: Grey Moore Date: Fri, 30 Jun 2023 11:40:40 -0400 Subject: [PATCH 06/62] rename requeue_strategy to requeue_with --- lib/sidekiq/throttled/configuration.rb | 8 +++--- lib/sidekiq/throttled/strategy.rb | 22 ++++++++-------- spec/lib/sidekiq/throttled/strategy_spec.rb | 28 ++++++++++----------- 3 files changed, 29 insertions(+), 29 deletions(-) diff --git a/lib/sidekiq/throttled/configuration.rb b/lib/sidekiq/throttled/configuration.rb index 1bee4427..62ec3901 100644 --- a/lib/sidekiq/throttled/configuration.rb +++ b/lib/sidekiq/throttled/configuration.rb @@ -4,7 +4,7 @@ module Sidekiq module Throttled # Configuration holder. class Configuration - attr_reader :default_requeue_strategy + attr_reader :default_requeue_with # Class constructor. def initialize @@ -16,7 +16,7 @@ def initialize # @return [self] def reset! @inherit_strategies = false - @default_requeue_strategy = :enqueue + @default_requeue_with = :enqueue self end @@ -53,8 +53,8 @@ def inherit_strategies? # Options are `:enqueue` (put them on the end of the queue) and `:schedule` (schedule for later). # Default: `:enqueue` # - def default_requeue_strategy=(value) - @default_requeue_strategy = value.intern + def default_requeue_with=(value) + @default_requeue_with = value.intern end end end diff --git a/lib/sidekiq/throttled/strategy.rb b/lib/sidekiq/throttled/strategy.rb index 04ce33b8..20fcd471 100644 --- a/lib/sidekiq/throttled/strategy.rb +++ b/lib/sidekiq/throttled/strategy.rb @@ -24,11 +24,11 @@ class Strategy # @return [Proc, nil] attr_reader :observer - # @!attribute [r] requeue_strategy + # @!attribute [r] requeue_with # @return [String] - attr_reader :requeue_strategy + attr_reader :requeue_with - REQUEUE_STRATEGIES = %i[enqueue schedule].freeze + VALID_VALUES_FOR_REQUEUE_WITH = %i[enqueue schedule].freeze # @param [#to_s] name # @param [Hash] concurrency Concurrency options. @@ -37,10 +37,10 @@ class Strategy # See keyword args of {Strategy::Threshold#initialize} for details. # @param [#call] key_suffix Dynamic key suffix generator. # @param [#call] observer Process called after throttled. - # @param [#to_s] requeue_strategy What to do with jobs that are throttled - def initialize(name, concurrency: nil, threshold: nil, key_suffix: nil, observer: nil, requeue_strategy: nil) # rubocop:disable Metrics/MethodLength, Metrics/ParameterLists + # @param [#to_s] requeue_with What to do with jobs that are throttled + def initialize(name, concurrency: nil, threshold: nil, key_suffix: nil, observer: nil, requeue_with: nil) # rubocop:disable Metrics/MethodLength, Metrics/ParameterLists @observer = observer - @requeue_strategy = requeue_strategy || Throttled.configuration.default_requeue_strategy + @requeue_with = requeue_with || Throttled.configuration.default_requeue_with @concurrency = StrategyCollection.new(concurrency, strategy: Concurrency, @@ -54,9 +54,9 @@ def initialize(name, concurrency: nil, threshold: nil, key_suffix: nil, observer raise ArgumentError, "Neither :concurrency nor :threshold given" unless @concurrency.any? || @threshold.any? - return if REQUEUE_STRATEGIES.include?(@requeue_strategy) + return if VALID_VALUES_FOR_REQUEUE_WITH.include?(@requeue_with) - raise ArgumentError, "#{requeue_strategy} is not a valid :requeue_strategy" + raise ArgumentError, "#{requeue_with} is not a valid value for :requeue_with" end # @return [Boolean] whenever strategy has dynamic config @@ -84,11 +84,11 @@ def throttled?(jid, *job_args) false end - # Return throttled job to be executed later. Implementation depends on the requeue_strategy. + # Return throttled job to be executed later. Implementation depends on the value of requeue_with. # # @return [void] def requeue_throttled(work) - case requeue_strategy + case requeue_with when :enqueue # Push the job back to the head of the queue. # This is the same operation Sidekiq performs upon `Sidekiq::Worker.perform_async` call. @@ -97,7 +97,7 @@ def requeue_throttled(work) # Find out when we will next be able to execute this job, and reschedule for then. reschedule_throttled(work) else - raise "unrecognized requeue_strategy #{requeue_strategy}" + raise "unrecognized requeue_with option #{requeue_with}" end end diff --git a/spec/lib/sidekiq/throttled/strategy_spec.rb b/spec/lib/sidekiq/throttled/strategy_spec.rb index 3dd8d91e..b5d76e01 100644 --- a/spec/lib/sidekiq/throttled/strategy_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy_spec.rb @@ -46,23 +46,23 @@ def perform(*); end }) end - it "uses :enqueue requeue_strategy by default" do + it "uses :enqueue requeue_with by default" do key_suffix = ->(_) {} instance = described_class.new(:foo, threshold: { limit: 123, period: 456, key_suffix: key_suffix }) - expect(instance.requeue_strategy).to eq :enqueue + expect(instance.requeue_with).to eq :enqueue end - it "uses specified requeue_strategy" do + it "uses specified requeue_with" do key_suffix = ->(_) {} instance = described_class.new(:foo, threshold: { limit: 123, period: 456, key_suffix: key_suffix }, - requeue_strategy: :schedule) - expect(instance.requeue_strategy).to eq :schedule + requeue_with: :schedule) + expect(instance.requeue_with).to eq :schedule end - context "when a default_requeue_strategy is set" do - before { Sidekiq::Throttled.configuration.default_requeue_strategy = :schedule } + context "when a default_requeue_with is set" do + before { Sidekiq::Throttled.configuration.default_requeue_with = :schedule } after { Sidekiq::Throttled.configuration.reset! } @@ -70,15 +70,15 @@ def perform(*); end key_suffix = ->(_) {} instance = described_class.new(:foo, threshold: { limit: 123, period: 456, key_suffix: key_suffix }) - expect(instance.requeue_strategy).to eq :schedule + expect(instance.requeue_with).to eq :schedule end it "allows overriding the default" do key_suffix = ->(_) {} instance = described_class.new(:foo, threshold: { limit: 123, period: 456, key_suffix: key_suffix }, - requeue_strategy: :enqueue) - expect(instance.requeue_strategy).to eq :enqueue + requeue_with: :enqueue) + expect(instance.requeue_with).to eq :enqueue end end end @@ -263,7 +263,7 @@ def perform(*); end Sidekiq::BasicFetch::UnitOfWork.new("queue:default", job, sidekiq_config) end - context "with requeue_strategy = :enqueue" do + context "with requeue_with = :enqueue" do def enqueued_jobs(queue) Sidekiq.redis do |conn| conn.lrange("queue:#{queue}", 0, -1).map do |job| @@ -273,7 +273,7 @@ def enqueued_jobs(queue) end end end - let(:options) { threshold.merge(requeue_strategy: :enqueue) } + let(:options) { threshold.merge(requeue_with: :enqueue) } it "puts the job back on the queue" do expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) @@ -287,8 +287,8 @@ def enqueued_jobs(queue) end end - context "with requeue_strategy = :schedule" do - let(:options) { basic_options.merge(requeue_strategy: :schedule) } + context "with requeue_with = :schedule" do + let(:options) { basic_options.merge(requeue_with: :schedule) } context "when threshold constraints given" do let(:basic_options) { threshold } From 37c905a40e2554303688ca7ceaac1c25b1dc5dec Mon Sep 17 00:00:00 2001 From: Grey Moore Date: Fri, 30 Jun 2023 15:12:31 -0400 Subject: [PATCH 07/62] store requeue_with via sidekiq_class_attribute, rather than in the Strategy --- lib/sidekiq/throttled.rb | 4 +- lib/sidekiq/throttled/job.rb | 15 ++- lib/sidekiq/throttled/strategy.rb | 21 ++-- spec/lib/sidekiq/throttled/job_spec.rb | 42 +++++++- spec/lib/sidekiq/throttled/strategy_spec.rb | 111 ++++++-------------- spec/lib/sidekiq/throttled_spec.rb | 33 ++++++ 6 files changed, 129 insertions(+), 97 deletions(-) diff --git a/lib/sidekiq/throttled.rb b/lib/sidekiq/throttled.rb index dd47902c..39b6b36d 100644 --- a/lib/sidekiq/throttled.rb +++ b/lib/sidekiq/throttled.rb @@ -77,10 +77,10 @@ def throttled?(message) # @return [void] def requeue_throttled(work) message = JSON.parse(work.job) - job_class = message.fetch("wrapped") { message.fetch("class") { return false } } + job_class = Object.const_get(message.fetch("wrapped") { message.fetch("class") { return false } }) Registry.get job_class do |strategy| - strategy.requeue_throttled(work) + strategy.requeue_throttled(work, requeue_with: job_class.sidekiq_throttled_requeue_with) end end end diff --git a/lib/sidekiq/throttled/job.rb b/lib/sidekiq/throttled/job.rb index a1a173be..96a6bdfb 100644 --- a/lib/sidekiq/throttled/job.rb +++ b/lib/sidekiq/throttled/job.rb @@ -14,7 +14,7 @@ module Throttled # include Sidekiq::Throttled::Job # # sidkiq_options :queue => :my_queue - # sidekiq_throttle :threshold => { :limit => 123, :period => 1.hour } + # sidekiq_throttle :threshold => { :limit => 123, :period => 1.hour }, :requeue_with => :schedule # # def perform # # ... @@ -23,6 +23,8 @@ module Throttled # # @see ClassMethods module Job + VALID_VALUES_FOR_REQUEUE_WITH = %i[enqueue schedule].freeze + # Extends worker class with {ClassMethods}. # # @note Using `included` hook with extending worker with {ClassMethods} @@ -30,6 +32,7 @@ module Job # # @private def self.included(worker) + worker.sidekiq_class_attribute :sidekiq_throttled_requeue_with # :enqueue | :schedule worker.send(:extend, ClassMethods) end @@ -71,9 +74,17 @@ module ClassMethods # }) # end # - # @see Registry.add + # @param [#to_s] requeue_with What to do with jobs that are throttled + # @see Registry.add for other parameters # @return [void] def sidekiq_throttle(**kwargs) + requeue_with = kwargs.delete(:requeue_with) || Throttled.configuration.default_requeue_with + unless VALID_VALUES_FOR_REQUEUE_WITH.include?(requeue_with) + raise ArgumentError, "#{requeue_with} is not a valid value for :requeue_with" + end + + self.sidekiq_throttled_requeue_with = requeue_with + Registry.add(self, **kwargs) end diff --git a/lib/sidekiq/throttled/strategy.rb b/lib/sidekiq/throttled/strategy.rb index 20fcd471..dc386ec9 100644 --- a/lib/sidekiq/throttled/strategy.rb +++ b/lib/sidekiq/throttled/strategy.rb @@ -24,12 +24,6 @@ class Strategy # @return [Proc, nil] attr_reader :observer - # @!attribute [r] requeue_with - # @return [String] - attr_reader :requeue_with - - VALID_VALUES_FOR_REQUEUE_WITH = %i[enqueue schedule].freeze - # @param [#to_s] name # @param [Hash] concurrency Concurrency options. # See keyword args of {Strategy::Concurrency#initialize} for details. @@ -37,10 +31,8 @@ class Strategy # See keyword args of {Strategy::Threshold#initialize} for details. # @param [#call] key_suffix Dynamic key suffix generator. # @param [#call] observer Process called after throttled. - # @param [#to_s] requeue_with What to do with jobs that are throttled - def initialize(name, concurrency: nil, threshold: nil, key_suffix: nil, observer: nil, requeue_with: nil) # rubocop:disable Metrics/MethodLength, Metrics/ParameterLists + def initialize(name, concurrency: nil, threshold: nil, key_suffix: nil, observer: nil) @observer = observer - @requeue_with = requeue_with || Throttled.configuration.default_requeue_with @concurrency = StrategyCollection.new(concurrency, strategy: Concurrency, @@ -53,10 +45,6 @@ def initialize(name, concurrency: nil, threshold: nil, key_suffix: nil, observer key_suffix: key_suffix) raise ArgumentError, "Neither :concurrency nor :threshold given" unless @concurrency.any? || @threshold.any? - - return if VALID_VALUES_FOR_REQUEUE_WITH.include?(@requeue_with) - - raise ArgumentError, "#{requeue_with} is not a valid value for :requeue_with" end # @return [Boolean] whenever strategy has dynamic config @@ -84,10 +72,13 @@ def throttled?(jid, *job_args) false end - # Return throttled job to be executed later. Implementation depends on the value of requeue_with. + # Return throttled job to be executed later. Implementation depends on the value of requeue_with: + # :enqueue means put the job back at the end of the queue immediately + # :schedule means schedule enqueueing the job for a later time when we expect to have capacity # + # @param [#to_s] requeue_with How to handle the throttled job # @return [void] - def requeue_throttled(work) + def requeue_throttled(work, requeue_with:) case requeue_with when :enqueue # Push the job back to the head of the queue. diff --git a/spec/lib/sidekiq/throttled/job_spec.rb b/spec/lib/sidekiq/throttled/job_spec.rb index fdb4d318..aa663e3f 100644 --- a/spec/lib/sidekiq/throttled/job_spec.rb +++ b/spec/lib/sidekiq/throttled/job_spec.rb @@ -1,7 +1,12 @@ # frozen_string_literal: true RSpec.describe Sidekiq::Throttled::Job do - let(:working_class) { Class.new { include Sidekiq::Throttled::Job } } + let(:working_class) do + Class.new do + include Sidekiq::Job + include Sidekiq::Throttled::Job + end + end it "aliased as Sidekiq::Throttled::Worker" do expect(Sidekiq::Throttled::Worker).to be described_class @@ -13,6 +18,41 @@ .to receive(:add).with(working_class, foo: :bar) working_class.sidekiq_throttle(foo: :bar) + + expect(working_class.sidekiq_throttled_requeue_with).to eq :enqueue + end + + it "accepts and stores a requeue_with parameter" do + expect(Sidekiq::Throttled::Registry) + .to receive(:add).with(working_class, foo: :bar) + + working_class.sidekiq_throttle(foo: :bar, requeue_with: :schedule) + + expect(working_class.sidekiq_throttled_requeue_with).to eq :schedule + end + + context "when a default_requeue_with is set" do + before { Sidekiq::Throttled.configuration.default_requeue_with = :schedule } + + after { Sidekiq::Throttled.configuration.reset! } + + it "uses the default when not overridden" do + expect(Sidekiq::Throttled::Registry) + .to receive(:add).with(working_class, foo: :bar) + + working_class.sidekiq_throttle(foo: :bar) + + expect(working_class.sidekiq_throttled_requeue_with).to eq :schedule + end + + it "allows overriding the default" do + expect(Sidekiq::Throttled::Registry) + .to receive(:add).with(working_class, foo: :bar) + + working_class.sidekiq_throttle(foo: :bar, requeue_with: :enqueue) + + expect(working_class.sidekiq_throttled_requeue_with).to eq :enqueue + end end end diff --git a/spec/lib/sidekiq/throttled/strategy_spec.rb b/spec/lib/sidekiq/throttled/strategy_spec.rb index b5d76e01..2a3e83ec 100644 --- a/spec/lib/sidekiq/throttled/strategy_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy_spec.rb @@ -45,42 +45,6 @@ def perform(*); end key_suffix: key_suffix }) end - - it "uses :enqueue requeue_with by default" do - key_suffix = ->(_) {} - - instance = described_class.new(:foo, threshold: { limit: 123, period: 456, key_suffix: key_suffix }) - expect(instance.requeue_with).to eq :enqueue - end - - it "uses specified requeue_with" do - key_suffix = ->(_) {} - - instance = described_class.new(:foo, threshold: { limit: 123, period: 456, key_suffix: key_suffix }, - requeue_with: :schedule) - expect(instance.requeue_with).to eq :schedule - end - - context "when a default_requeue_with is set" do - before { Sidekiq::Throttled.configuration.default_requeue_with = :schedule } - - after { Sidekiq::Throttled.configuration.reset! } - - it "uses the default when not overridden" do - key_suffix = ->(_) {} - - instance = described_class.new(:foo, threshold: { limit: 123, period: 456, key_suffix: key_suffix }) - expect(instance.requeue_with).to eq :schedule - end - - it "allows overriding the default" do - key_suffix = ->(_) {} - - instance = described_class.new(:foo, threshold: { limit: 123, period: 456, key_suffix: key_suffix }, - requeue_with: :enqueue) - expect(instance.requeue_with).to eq :enqueue - end - end end describe "#throttled?" do @@ -263,7 +227,9 @@ def perform(*); end Sidekiq::BasicFetch::UnitOfWork.new("queue:default", job, sidekiq_config) end - context "with requeue_with = :enqueue" do + describe "with requeue_with = :enqueue" do + let(:options) { threshold } + def enqueued_jobs(queue) Sidekiq.redis do |conn| conn.lrange("queue:#{queue}", 0, -1).map do |job| @@ -273,13 +239,12 @@ def enqueued_jobs(queue) end end end - let(:options) { threshold.merge(requeue_with: :enqueue) } it "puts the job back on the queue" do expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) # Requeue the work - subject.requeue_throttled(work) + subject.requeue_throttled(work, requeue_with: :enqueue) # See that it is now on the end of the queue expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], @@ -287,11 +252,21 @@ def enqueued_jobs(queue) end end - context "with requeue_with = :schedule" do - let(:options) { basic_options.merge(requeue_with: :schedule) } + describe "with requeue_with = :schedule" do + def scheduled_redis_item_and_score + Sidekiq.redis do |conn| + # Depending on whether we have redis-client (for Sidekiq 7) or redis-rb (for older Sidekiq), + # zscan takes different arguments + if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") + conn.zscan("schedule", 0).last.first + else + conn.zscan("schedule").first + end + end + end context "when threshold constraints given" do - let(:basic_options) { threshold } + let(:options) { threshold } before do allow(subject.threshold).to receive(:retry_in).and_return(300.0) @@ -299,24 +274,18 @@ def enqueued_jobs(queue) it "reschedules for when the threshold strategy says to, plus some jitter" do # Requeue the work, see that it ends up in 'schedule' - expect { subject.requeue_throttled(work) }.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) - - item, score = Sidekiq.redis do |conn| - # Depending on whether we have redis-client (for Sidekiq 7) or redis-rb (for older Sidekiq), - # zscan takes different arguments - if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") - conn.zscan("schedule", 0).last.first - else - conn.zscan("schedule").first - end - end + expect do + subject.requeue_throttled(work, requeue_with: :schedule) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) end end context "when concurrency constraints given" do - let(:basic_options) { concurrency } + let(:options) { concurrency } before do allow(subject.concurrency).to receive(:retry_in).and_return(300.0) @@ -324,24 +293,18 @@ def enqueued_jobs(queue) it "reschedules for when the concurrency strategy says to, plus some jitter" do # Requeue the work, see that it ends up in 'schedule' - expect { subject.requeue_throttled(work) }.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) - - item, score = Sidekiq.redis do |conn| - # Depending on whether we have redis-client (for Sidekiq 7) or redis-rb (for older Sidekiq), - # zscan takes different arguments - if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") - conn.zscan("schedule", 0).last.first - else - conn.zscan("schedule").first - end - end + expect do + subject.requeue_throttled(work, requeue_with: :schedule) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) end end context "when threshold and concurrency constraints given" do - let(:basic_options) { threshold.merge concurrency } + let(:options) { threshold.merge concurrency } before do allow(subject.concurrency).to receive(:retry_in).and_return(300.0) @@ -350,17 +313,11 @@ def enqueued_jobs(queue) it "reschedules for the later of what the two say, plus some jitter" do # Requeue the work, see that it ends up in 'schedule' - expect { subject.requeue_throttled(work) }.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) - - item, score = Sidekiq.redis do |conn| - # Depending on whether we have redis-client (for Sidekiq 7) or redis-rb (for older Sidekiq), - # zscan takes different arguments - if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") - conn.zscan("schedule", 0).last.first - else - conn.zscan("schedule").first - end - end + expect do + subject.requeue_throttled(work, requeue_with: :schedule) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") expect(score.to_f).to be_within(51.0).of(Time.now.to_f + 550.0) end diff --git a/spec/lib/sidekiq/throttled_spec.rb b/spec/lib/sidekiq/throttled_spec.rb index 970122f8..d8527d09 100644 --- a/spec/lib/sidekiq/throttled_spec.rb +++ b/spec/lib/sidekiq/throttled_spec.rb @@ -2,6 +2,13 @@ require "json" +class ThrottledTestJob + include Sidekiq::Job + include Sidekiq::Throttled::Job + + def perform(*); end +end + RSpec.describe Sidekiq::Throttled do describe ".setup!" do before do @@ -86,4 +93,30 @@ described_class.throttled? message end end + + describe ".requeue_throttled" do + let(:sidekiq_config) do + if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") + Sidekiq::DEFAULTS + else + Sidekiq::Config.new(queues: %w[default]).default_capsule + end + end + + let!(:strategy) { Sidekiq::Throttled::Registry.add("ThrottledTestJob", threshold: { limit: 1, period: 1 }) } + + before do + ThrottledTestJob.sidekiq_throttled_requeue_with = :enqueue + end + + it "invokes requeue_throttled on the strategy" do + payload_jid = jid + job = { class: "ThrottledTestJob", jid: payload_jid.inspect }.to_json + work = Sidekiq::BasicFetch::UnitOfWork.new("queue:default", job, sidekiq_config) + + expect(strategy).to receive(:requeue_throttled).with(work, requeue_with: :enqueue) + + described_class.requeue_throttled work + end + end end From ce5745d18dd41016c61c132c94b312d69b18ee97 Mon Sep 17 00:00:00 2001 From: Grey Moore Date: Fri, 30 Jun 2023 16:46:01 -0400 Subject: [PATCH 08/62] allow specifying which queue throttled jobs should be sent to --- lib/sidekiq/throttled.rb | 2 +- lib/sidekiq/throttled/configuration.rb | 17 ++++--- lib/sidekiq/throttled/job.rb | 27 ++++++++--- lib/sidekiq/throttled/strategy.rb | 22 +++++---- spec/lib/sidekiq/throttled/job_spec.rb | 54 +++++++++++++++++---- spec/lib/sidekiq/throttled/strategy_spec.rb | 38 ++++++++++++--- spec/lib/sidekiq/throttled_spec.rb | 4 +- 7 files changed, 124 insertions(+), 40 deletions(-) diff --git a/lib/sidekiq/throttled.rb b/lib/sidekiq/throttled.rb index 39b6b36d..4f3e4ab6 100644 --- a/lib/sidekiq/throttled.rb +++ b/lib/sidekiq/throttled.rb @@ -80,7 +80,7 @@ def requeue_throttled(work) job_class = Object.const_get(message.fetch("wrapped") { message.fetch("class") { return false } }) Registry.get job_class do |strategy| - strategy.requeue_throttled(work, requeue_with: job_class.sidekiq_throttled_requeue_with) + strategy.requeue_throttled(work, **job_class.sidekiq_throttled_requeue_options) end end end diff --git a/lib/sidekiq/throttled/configuration.rb b/lib/sidekiq/throttled/configuration.rb index 62ec3901..a64a069c 100644 --- a/lib/sidekiq/throttled/configuration.rb +++ b/lib/sidekiq/throttled/configuration.rb @@ -4,7 +4,7 @@ module Sidekiq module Throttled # Configuration holder. class Configuration - attr_reader :default_requeue_with + attr_reader :default_requeue_options # Class constructor. def initialize @@ -16,7 +16,7 @@ def initialize # @return [self] def reset! @inherit_strategies = false - @default_requeue_with = :enqueue + @default_requeue_options = { with: :enqueue } self end @@ -50,11 +50,16 @@ def inherit_strategies? end # Specifies how we should return throttled jobs to the queue so they can be executed later. - # Options are `:enqueue` (put them on the end of the queue) and `:schedule` (schedule for later). - # Default: `:enqueue` + # Expects a hash with keys that may include :with and :to + # For :with, options are `:enqueue` (put them on the end of the queue) and `:schedule` (schedule for later). + # For :to, the name of a sidekiq queue should be specified. If none is specified, jobs will by default be + # requeued to the same queue they were originally enqueued in. + # Default: {with: `:enqueue`} # - def default_requeue_with=(value) - @default_requeue_with = value.intern + def default_requeue_options=(options) + requeue_with = options.delete(:with).intern || :enqueue + + @default_requeue_options = options.merge({ with: requeue_with }) end end end diff --git a/lib/sidekiq/throttled/job.rb b/lib/sidekiq/throttled/job.rb index 96a6bdfb..5b4f66f5 100644 --- a/lib/sidekiq/throttled/job.rb +++ b/lib/sidekiq/throttled/job.rb @@ -14,7 +14,8 @@ module Throttled # include Sidekiq::Throttled::Job # # sidkiq_options :queue => :my_queue - # sidekiq_throttle :threshold => { :limit => 123, :period => 1.hour }, :requeue_with => :schedule + # sidekiq_throttle :threshold => { :limit => 123, :period => 1.hour }, + # :requeue => { :to => :other_queue, :with => :schedule } # # def perform # # ... @@ -32,7 +33,7 @@ module Job # # @private def self.included(worker) - worker.sidekiq_class_attribute :sidekiq_throttled_requeue_with # :enqueue | :schedule + worker.sidekiq_class_attribute :sidekiq_throttled_requeue_options worker.send(:extend, ClassMethods) end @@ -74,16 +75,28 @@ module ClassMethods # }) # end # - # @param [#to_s] requeue_with What to do with jobs that are throttled + # @example Allow max 123 MyJob jobs per hour, and when jobs are throttled, schedule them for later in :other_queue + # + # class MyJob + # include Sidekiq::Job + # include Sidekiq::Throttled::Job + # + # sidekiq_throttle({ + # :threshold => { :limit => 123, :period => 1.hour }, + # :requeue => { :to => :other_queue, :with => :schedule } + # }) + # end + # + # @param [Hash] requeue What to do with jobs that are throttled # @see Registry.add for other parameters # @return [void] def sidekiq_throttle(**kwargs) - requeue_with = kwargs.delete(:requeue_with) || Throttled.configuration.default_requeue_with - unless VALID_VALUES_FOR_REQUEUE_WITH.include?(requeue_with) - raise ArgumentError, "#{requeue_with} is not a valid value for :requeue_with" + requeue_options = Throttled.configuration.default_requeue_options.merge(kwargs.delete(:requeue) || {}) + unless VALID_VALUES_FOR_REQUEUE_WITH.include?(requeue_options[:with]) + raise ArgumentError, "requeue: #{requeue_options[:with]} is not a valid value for :with" end - self.sidekiq_throttled_requeue_with = requeue_with + self.sidekiq_throttled_requeue_options = requeue_options Registry.add(self, **kwargs) end diff --git a/lib/sidekiq/throttled/strategy.rb b/lib/sidekiq/throttled/strategy.rb index dc386ec9..77593009 100644 --- a/lib/sidekiq/throttled/strategy.rb +++ b/lib/sidekiq/throttled/strategy.rb @@ -72,23 +72,26 @@ def throttled?(jid, *job_args) false end - # Return throttled job to be executed later. Implementation depends on the value of requeue_with: + # Return throttled job to be executed later. Implementation depends on the value of `with`: # :enqueue means put the job back at the end of the queue immediately # :schedule means schedule enqueueing the job for a later time when we expect to have capacity # - # @param [#to_s] requeue_with How to handle the throttled job + # @param [#to_s] with How to handle the throttled job + # @param [#to_s] to Name of the queue to re-queue the job to. If not specified, will use the job's original queue. # @return [void] - def requeue_throttled(work, requeue_with:) - case requeue_with + def requeue_throttled(work, with:, to: nil) + case with when :enqueue # Push the job back to the head of the queue. + target_list = to.nil? ? work.queue : "queue:#{to}" + # This is the same operation Sidekiq performs upon `Sidekiq::Worker.perform_async` call. - Sidekiq.redis { |conn| conn.lpush(work.queue, work.job) } + Sidekiq.redis { |conn| conn.lpush(target_list, work.job) } when :schedule # Find out when we will next be able to execute this job, and reschedule for then. - reschedule_throttled(work) + reschedule_throttled(work, to: to) else - raise "unrecognized requeue_with option #{requeue_with}" + raise "unrecognized :with option #{with}" end end @@ -107,12 +110,13 @@ def reset! private - def reschedule_throttled(work) + def reschedule_throttled(work, to: nil) message = JSON.parse(work.job) job_class = message.fetch("wrapped") { message.fetch("class") { return false } } job_args = message["args"] - Sidekiq::Client.enqueue_to_in(work.queue, retry_in(work), Object.const_get(job_class), *job_args) + target_queue = to.nil? ? work.queue : "queue:#{to}" + Sidekiq::Client.enqueue_to_in(target_queue, retry_in(work), Object.const_get(job_class), *job_args) end def retry_in(work) diff --git a/spec/lib/sidekiq/throttled/job_spec.rb b/spec/lib/sidekiq/throttled/job_spec.rb index aa663e3f..9974c21e 100644 --- a/spec/lib/sidekiq/throttled/job_spec.rb +++ b/spec/lib/sidekiq/throttled/job_spec.rb @@ -19,20 +19,38 @@ working_class.sidekiq_throttle(foo: :bar) - expect(working_class.sidekiq_throttled_requeue_with).to eq :enqueue + expect(working_class.sidekiq_throttled_requeue_options).to eq({ with: :enqueue }) end - it "accepts and stores a requeue_with parameter" do + it "accepts and stores a requeue parameter including :with" do expect(Sidekiq::Throttled::Registry) .to receive(:add).with(working_class, foo: :bar) - working_class.sidekiq_throttle(foo: :bar, requeue_with: :schedule) + working_class.sidekiq_throttle(foo: :bar, requeue: { with: :schedule }) - expect(working_class.sidekiq_throttled_requeue_with).to eq :schedule + expect(working_class.sidekiq_throttled_requeue_options).to eq({ with: :schedule }) end - context "when a default_requeue_with is set" do - before { Sidekiq::Throttled.configuration.default_requeue_with = :schedule } + it "accepts and stores a requeue parameter including :to" do + expect(Sidekiq::Throttled::Registry) + .to receive(:add).with(working_class, foo: :bar) + + working_class.sidekiq_throttle(foo: :bar, requeue: { to: :other_queue }) + + expect(working_class.sidekiq_throttled_requeue_options).to eq({ to: :other_queue, with: :enqueue }) + end + + it "accepts and stores a requeue parameter including both :to and :with" do + expect(Sidekiq::Throttled::Registry) + .to receive(:add).with(working_class, foo: :bar) + + working_class.sidekiq_throttle(foo: :bar, requeue: { to: :other_queue, with: :schedule }) + + expect(working_class.sidekiq_throttled_requeue_options).to eq({ to: :other_queue, with: :schedule }) + end + + context "when default_requeue_options are set" do + before { Sidekiq::Throttled.configuration.default_requeue_options = { with: :schedule } } after { Sidekiq::Throttled.configuration.reset! } @@ -42,16 +60,34 @@ working_class.sidekiq_throttle(foo: :bar) - expect(working_class.sidekiq_throttled_requeue_with).to eq :schedule + expect(working_class.sidekiq_throttled_requeue_options).to eq({ with: :schedule }) + end + + it "uses the default alongside a requeue parameter including :to" do + expect(Sidekiq::Throttled::Registry) + .to receive(:add).with(working_class, foo: :bar) + + working_class.sidekiq_throttle(foo: :bar, requeue: { to: :other_queue }) + + expect(working_class.sidekiq_throttled_requeue_options).to eq({ to: :other_queue, with: :schedule }) end it "allows overriding the default" do expect(Sidekiq::Throttled::Registry) .to receive(:add).with(working_class, foo: :bar) - working_class.sidekiq_throttle(foo: :bar, requeue_with: :enqueue) + working_class.sidekiq_throttle(foo: :bar, requeue: { with: :enqueue }) + + expect(working_class.sidekiq_throttled_requeue_options).to eq({ with: :enqueue }) + end + + it "allows overriding the default and including a :to parameter" do + expect(Sidekiq::Throttled::Registry) + .to receive(:add).with(working_class, foo: :bar) + + working_class.sidekiq_throttle(foo: :bar, requeue: { to: :other_queue, with: :enqueue }) - expect(working_class.sidekiq_throttled_requeue_with).to eq :enqueue + expect(working_class.sidekiq_throttled_requeue_options).to eq({ to: :other_queue, with: :enqueue }) end end end diff --git a/spec/lib/sidekiq/throttled/strategy_spec.rb b/spec/lib/sidekiq/throttled/strategy_spec.rb index 2a3e83ec..23252045 100644 --- a/spec/lib/sidekiq/throttled/strategy_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy_spec.rb @@ -227,7 +227,7 @@ def perform(*); end Sidekiq::BasicFetch::UnitOfWork.new("queue:default", job, sidekiq_config) end - describe "with requeue_with = :enqueue" do + describe "with parameter with: :enqueue" do let(:options) { threshold } def enqueued_jobs(queue) @@ -242,17 +242,31 @@ def enqueued_jobs(queue) it "puts the job back on the queue" do expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty # Requeue the work - subject.requeue_throttled(work, requeue_with: :enqueue) + subject.requeue_throttled(work, with: :enqueue) # See that it is now on the end of the queue expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty + end + + it "puts the job back on a different queue when specified" do + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty + + # Requeue the work + subject.requeue_throttled(work, with: :enqueue, to: :other_queue) + + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to eq([["ThrottledTestJob", [1]]]) end end - describe "with requeue_with = :schedule" do + describe "with parameter with: :schedule" do def scheduled_redis_item_and_score Sidekiq.redis do |conn| # Depending on whether we have redis-client (for Sidekiq 7) or redis-rb (for older Sidekiq), @@ -275,13 +289,25 @@ def scheduled_redis_item_and_score it "reschedules for when the threshold strategy says to, plus some jitter" do # Requeue the work, see that it ends up in 'schedule' expect do - subject.requeue_throttled(work, requeue_with: :schedule) + subject.requeue_throttled(work, with: :schedule) end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) item, score = scheduled_redis_item_and_score expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) end + + it "reschedules for a different queue if specified" do + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: :schedule, to: :other_queue) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:other_queue") + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + end end context "when concurrency constraints given" do @@ -294,7 +320,7 @@ def scheduled_redis_item_and_score it "reschedules for when the concurrency strategy says to, plus some jitter" do # Requeue the work, see that it ends up in 'schedule' expect do - subject.requeue_throttled(work, requeue_with: :schedule) + subject.requeue_throttled(work, with: :schedule) end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) item, score = scheduled_redis_item_and_score @@ -314,7 +340,7 @@ def scheduled_redis_item_and_score it "reschedules for the later of what the two say, plus some jitter" do # Requeue the work, see that it ends up in 'schedule' expect do - subject.requeue_throttled(work, requeue_with: :schedule) + subject.requeue_throttled(work, with: :schedule) end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) item, score = scheduled_redis_item_and_score diff --git a/spec/lib/sidekiq/throttled_spec.rb b/spec/lib/sidekiq/throttled_spec.rb index d8527d09..4c800007 100644 --- a/spec/lib/sidekiq/throttled_spec.rb +++ b/spec/lib/sidekiq/throttled_spec.rb @@ -106,7 +106,7 @@ def perform(*); end let!(:strategy) { Sidekiq::Throttled::Registry.add("ThrottledTestJob", threshold: { limit: 1, period: 1 }) } before do - ThrottledTestJob.sidekiq_throttled_requeue_with = :enqueue + ThrottledTestJob.sidekiq_throttled_requeue_options = { to: :other_queue, with: :enqueue } end it "invokes requeue_throttled on the strategy" do @@ -114,7 +114,7 @@ def perform(*); end job = { class: "ThrottledTestJob", jid: payload_jid.inspect }.to_json work = Sidekiq::BasicFetch::UnitOfWork.new("queue:default", job, sidekiq_config) - expect(strategy).to receive(:requeue_throttled).with(work, requeue_with: :enqueue) + expect(strategy).to receive(:requeue_throttled).with(work, to: :other_queue, with: :enqueue) described_class.requeue_throttled work end From 122e6e1ebd316b3854800ad2873f9310a53ed23b Mon Sep 17 00:00:00 2001 From: Grey Moore Date: Fri, 30 Jun 2023 17:12:56 -0400 Subject: [PATCH 09/62] accept a Proc for :with and :to arguments --- lib/sidekiq/throttled/job.rb | 2 +- lib/sidekiq/throttled/strategy.rb | 22 ++++++---- spec/lib/sidekiq/throttled/strategy_spec.rb | 48 +++++++++++++++++++++ 3 files changed, 63 insertions(+), 9 deletions(-) diff --git a/lib/sidekiq/throttled/job.rb b/lib/sidekiq/throttled/job.rb index 5b4f66f5..3dc09b73 100644 --- a/lib/sidekiq/throttled/job.rb +++ b/lib/sidekiq/throttled/job.rb @@ -75,7 +75,7 @@ module ClassMethods # }) # end # - # @example Allow max 123 MyJob jobs per hour, and when jobs are throttled, schedule them for later in :other_queue + # @example Allow max 123 MyJob jobs per hour; when jobs are throttled, schedule them for later in :other_queue # # class MyJob # include Sidekiq::Job diff --git a/lib/sidekiq/throttled/strategy.rb b/lib/sidekiq/throttled/strategy.rb index 77593009..b5d6e810 100644 --- a/lib/sidekiq/throttled/strategy.rb +++ b/lib/sidekiq/throttled/strategy.rb @@ -76,20 +76,26 @@ def throttled?(jid, *job_args) # :enqueue means put the job back at the end of the queue immediately # :schedule means schedule enqueueing the job for a later time when we expect to have capacity # - # @param [#to_s] with How to handle the throttled job - # @param [#to_s] to Name of the queue to re-queue the job to. If not specified, will use the job's original queue. + # @param [#to_s, #call] with How to handle the throttled job + # @param [#to_s, #call] to Name of the queue to re-queue the job to. + # If not specified, will use the job's original queue. # @return [void] - def requeue_throttled(work, with:, to: nil) - case with + def requeue_throttled(work, with:, to: nil) # rubocop:disable Metrics/MethodLength + # Resolve :with and :to arguments, calling them if they are Procs + job_args = JSON.parse(work.job)["args"] + requeue_with = with.respond_to?(:call) ? with.call(*job_args) : with + requeue_to = to.respond_to?(:call) ? to.call(*job_args) : to + + case requeue_with when :enqueue # Push the job back to the head of the queue. - target_list = to.nil? ? work.queue : "queue:#{to}" + target_list = requeue_to.nil? ? work.queue : "queue:#{requeue_to}" # This is the same operation Sidekiq performs upon `Sidekiq::Worker.perform_async` call. Sidekiq.redis { |conn| conn.lpush(target_list, work.job) } when :schedule # Find out when we will next be able to execute this job, and reschedule for then. - reschedule_throttled(work, to: to) + reschedule_throttled(work, requeue_to: requeue_to) else raise "unrecognized :with option #{with}" end @@ -110,12 +116,12 @@ def reset! private - def reschedule_throttled(work, to: nil) + def reschedule_throttled(work, requeue_to: nil) message = JSON.parse(work.job) job_class = message.fetch("wrapped") { message.fetch("class") { return false } } job_args = message["args"] - target_queue = to.nil? ? work.queue : "queue:#{to}" + target_queue = requeue_to.nil? ? work.queue : "queue:#{requeue_to}" Sidekiq::Client.enqueue_to_in(target_queue, retry_in(work), Object.const_get(job_class), *job_args) end diff --git a/spec/lib/sidekiq/throttled/strategy_spec.rb b/spec/lib/sidekiq/throttled/strategy_spec.rb index 23252045..57f3ceea 100644 --- a/spec/lib/sidekiq/throttled/strategy_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy_spec.rb @@ -264,6 +264,31 @@ def enqueued_jobs(queue) expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) expect(enqueued_jobs("other_queue")).to eq([["ThrottledTestJob", [1]]]) end + + it "accepts a Proc for :with argument" do + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty + + # Requeue the work + subject.requeue_throttled(work, with: ->(_arg) { :enqueue }) + + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], + ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty + end + + it "accepts a Proc for :to argument" do + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty + + # Requeue the work + subject.requeue_throttled(work, with: :enqueue, to: ->(_arg) { :other_queue }) + + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to eq([["ThrottledTestJob", [1]]]) + end end describe "with parameter with: :schedule" do @@ -308,6 +333,29 @@ def scheduled_redis_item_and_score "queue" => "queue:other_queue") expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) end + + it "accepts a Proc for :with argument" do + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: ->(_arg) { :schedule }) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + end + + it "accepts a Proc for :to argument" do + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: :schedule, to: ->(_arg) { :other_queue }) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:other_queue") + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + end end context "when concurrency constraints given" do From dd0f7714edf0ec76b94678d38df30876878c4db1 Mon Sep 17 00:00:00 2001 From: Malav Bhavsar Date: Tue, 25 Jul 2023 19:46:24 -0700 Subject: [PATCH 10/62] fix: middleware not finalizing correctly when job is ActiveJob --- lib/sidekiq/throttled/middleware.rb | 9 ++++-- spec/lib/sidekiq/throttled/middleware_spec.rb | 30 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/lib/sidekiq/throttled/middleware.rb b/lib/sidekiq/throttled/middleware.rb index a5ed7444..54f777f9 100644 --- a/lib/sidekiq/throttled/middleware.rb +++ b/lib/sidekiq/throttled/middleware.rb @@ -15,8 +15,13 @@ class Middleware def call(_worker, msg, _queue) yield ensure - Registry.get msg["class"] do |strategy| - strategy.finalize!(msg["jid"], *msg["args"]) + job = msg.fetch("wrapped") { msg.fetch("class", false) } + jid = msg.fetch("jid", false) + + if job && jid + Registry.get job do |strategy| + strategy.finalize!(jid, *msg["args"]) + end end end end diff --git a/spec/lib/sidekiq/throttled/middleware_spec.rb b/spec/lib/sidekiq/throttled/middleware_spec.rb index 15ab2c4e..cbf21687 100644 --- a/spec/lib/sidekiq/throttled/middleware_spec.rb +++ b/spec/lib/sidekiq/throttled/middleware_spec.rb @@ -30,6 +30,36 @@ end end + context "when job class has strategy with concurrency constraint and uses ActiveJob" do + let(:payload) do + { + "class" => "ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper", + "wrapped" => "wrapped-foo", + "jid" => "bar" + } + end + + let! :strategy do + Sidekiq::Throttled::Registry.add payload["wrapped"], + concurrency: { limit: 1 } + end + + it "calls #finalize! of queue with jid of job being processed" do + expect(strategy).to receive(:finalize!).with "bar" + middleware.call(double, payload, double) { |*| } + end + + it "returns yields control to the given block" do + expect { |b| middleware.call(double, payload, double, &b) } + .to yield_control + end + + it "returns result of given block" do + expect(middleware.call(double, payload, double) { |*| :foobar }) + .to be :foobar + end + end + context "when job class has strategy without concurrency constraint" do let! :strategy do Sidekiq::Throttled::Registry.add payload["class"], From b1248446233ce113ab1f7842c4ca5af0847f223c Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Sun, 13 Aug 2023 00:43:33 +0200 Subject: [PATCH 11/62] Update CHANGES.md with #151 --- CHANGES.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 4b012655..d5714170 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Correctly finalize throttled jobs when used with ActiveJob + [#151](https://github.com/ixti/sidekiq-throttled/pull/151) + ## [1.0.0.alpha.1] - 2023-06-08 From 4bb625485df38d7da485735ff4205792d543262c Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Fri, 18 Aug 2023 04:33:37 +0200 Subject: [PATCH 12/62] lint: Fix rubocop rules --- .rubocop.yml | 4 ++++ rubocop/style.yml | 3 +++ 2 files changed, 7 insertions(+) diff --git a/.rubocop.yml b/.rubocop.yml index 98f0a84e..a2e7d7c8 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -19,3 +19,7 @@ AllCops: - vendor/**/* NewCops: enable TargetRubyVersion: 2.7 + +# Broken: https://github.com/rubocop/rubocop/issues/12113 +Bundler/DuplicatedGroup: + Enabled: false diff --git a/rubocop/style.yml b/rubocop/style.yml index 47b505f0..18f14c29 100644 --- a/rubocop/style.yml +++ b/rubocop/style.yml @@ -46,6 +46,9 @@ Style/OptionalBooleanParameter: Style/RedundantAssignment: Enabled: true +Style/RedundantCurrentDirectoryInPath: + Enabled: false + Style/RedundantFetchBlock: Enabled: true From b3221ea181394686d06f24ac31e643aaa9e9a44a Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 4 Sep 2023 17:08:20 +0000 Subject: [PATCH 13/62] Bump actions/checkout from 3 to 4 Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](https://github.com/actions/checkout/compare/v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 77d54a7d..fa4f911e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,7 +29,7 @@ jobs: --health-retries 5 steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - uses: ruby/setup-ruby@v1 with: @@ -43,7 +43,7 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - uses: ruby/setup-ruby@v1 with: From 37a1f1d9e737b254a52fb344f1802e553171ef54 Mon Sep 17 00:00:00 2001 From: Tomasz Nazar Date: Tue, 17 Oct 2023 16:52:11 +0200 Subject: [PATCH 14/62] Fix typo `sidkiq_options` (#158) --- lib/sidekiq/throttled/job.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/sidekiq/throttled/job.rb b/lib/sidekiq/throttled/job.rb index a1a173be..d7401333 100644 --- a/lib/sidekiq/throttled/job.rb +++ b/lib/sidekiq/throttled/job.rb @@ -13,7 +13,7 @@ module Throttled # include Sidekiq::Job # include Sidekiq::Throttled::Job # - # sidkiq_options :queue => :my_queue + # sidekiq_options :queue => :my_queue # sidekiq_throttle :threshold => { :limit => 123, :period => 1.hour } # # def perform From 7c0dbdbf0d060b85728ac123705e889df83be014 Mon Sep 17 00:00:00 2001 From: Yutaka Kamei Date: Tue, 17 Oct 2023 23:55:37 +0900 Subject: [PATCH 15/62] Replace "end" with "head" in test cases (#156) `basic_fetch_spec.rb` tests the behavior of requeue of `retrieve_work`. As far as I see, `requeue_throttled` uses Redis LPUSH, which inserts all the specified values at the head of the list stored at key. However, the previous test cases were telling "pushes job back to the end queue". I think this is not correct. When I was assessing this repository, I was a bit confused by the test case, so I want to correct the test case. --- spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb index 203daba2..78c4de13 100644 --- a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb +++ b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb @@ -57,7 +57,7 @@ def enqueued_jobs(queue) expect(fetch.retrieve_work).to be_nil end - it "pushes job back to the end queue" do + it "pushes job back to the head of the queue" do expect { fetch.retrieve_work } .to change { enqueued_jobs("default") } .to eq([["ThrottledTestJob", [2]], ["ThrottledTestJob", [3]]]) @@ -74,7 +74,7 @@ def enqueued_jobs(queue) expect(fetch.retrieve_work).to be_nil end - it "pushes job back to the end queue" do + it "pushes job back to the head of the queue" do expect { fetch.retrieve_work } .to change { enqueued_jobs("default") } .to eq([["ThrottledTestJob", [2]], ["ThrottledTestJob", [3]]]) From 479c06d31810c45023bdfc97b7d5926bc95dcf87 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Fri, 17 Nov 2023 16:37:42 +0100 Subject: [PATCH 16/62] cleanup: Drop ruby-2.7.x support --- .github/workflows/ci.yml | 2 +- .rubocop.yml | 2 +- CHANGES.md | 4 ++++ README.adoc | 1 - sidekiq-throttled.gemspec | 2 +- 5 files changed, 7 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fa4f911e..c1e19aba 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,7 +13,7 @@ jobs: strategy: fail-fast: false matrix: - ruby: [ "2.7", "3.0", "3.1", "3.2" ] + ruby: [ "3.0", "3.1", "3.2" ] runs-on: ubuntu-latest diff --git a/.rubocop.yml b/.rubocop.yml index a2e7d7c8..90e5e296 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -18,7 +18,7 @@ AllCops: - gemfiles/**/* - vendor/**/* NewCops: enable - TargetRubyVersion: 2.7 + TargetRubyVersion: 3.0 # Broken: https://github.com/rubocop/rubocop/issues/12113 Bundler/DuplicatedGroup: diff --git a/CHANGES.md b/CHANGES.md index d5714170..8e4e1d71 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -13,6 +13,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Correctly finalize throttled jobs when used with ActiveJob [#151](https://github.com/ixti/sidekiq-throttled/pull/151) +### Removed + +- (BREAKING) Drop Ruby-2.7.x support + ## [1.0.0.alpha.1] - 2023-06-08 diff --git a/README.adoc b/README.adoc index 21508cea..fc9e421f 100644 --- a/README.adoc +++ b/README.adoc @@ -232,7 +232,6 @@ sidekiq_throttle(concurrency: { limit: 20, ttl: 1.hour.to_i }) This library aims to support and is tested against the following Ruby versions: -* Ruby 2.7.x * Ruby 3.0.x * Ruby 3.1.x * Ruby 3.2.x diff --git a/sidekiq-throttled.gemspec b/sidekiq-throttled.gemspec index 46e01671..d67e5666 100644 --- a/sidekiq-throttled.gemspec +++ b/sidekiq-throttled.gemspec @@ -30,7 +30,7 @@ Gem::Specification.new do |spec| spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) } spec.require_paths = ["lib"] - spec.required_ruby_version = ">= 2.7" + spec.required_ruby_version = ">= 3.0" spec.add_runtime_dependency "redis-prescription", "~> 2.2" spec.add_runtime_dependency "sidekiq", ">= 6.5" From fa2d701b69694f14e5e18040f26d68c75d98bc9e Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Fri, 17 Nov 2023 16:40:22 +0100 Subject: [PATCH 17/62] cleanup: Drop sidekiq-6.x.x support --- Appraisals | 12 ++++++------ CHANGES.md | 1 + README.adoc | 2 +- lib/sidekiq/throttled/middleware.rb | 2 +- sidekiq-throttled.gemspec | 2 +- 5 files changed, 10 insertions(+), 9 deletions(-) diff --git a/Appraisals b/Appraisals index 235564ab..7e7c8815 100644 --- a/Appraisals +++ b/Appraisals @@ -1,11 +1,5 @@ # frozen_string_literal: true -appraise "sidekiq-6.5.x" do - group :test do - gem "sidekiq", "~> 6.5.0" - end -end - appraise "sidekiq-7.0.x" do group :test do gem "sidekiq", "~> 7.0.0" @@ -17,3 +11,9 @@ appraise "sidekiq-7.1.x" do gem "sidekiq", "~> 7.1.0" end end + +appraise "sidekiq-7.2.x" do + group :test do + gem "sidekiq", "~> 7.2.0" + end +end diff --git a/CHANGES.md b/CHANGES.md index 8e4e1d71..a4bd6ac5 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Removed - (BREAKING) Drop Ruby-2.7.x support +- (BREAKING) Drop Sidekiq-6.x.x support ## [1.0.0.alpha.1] - 2023-06-08 diff --git a/README.adoc b/README.adoc index fc9e421f..e704c168 100644 --- a/README.adoc +++ b/README.adoc @@ -254,9 +254,9 @@ dropped. This library aims to support and work with following Sidekiq versions: -* Sidekiq 6.5.x * Sidekiq 7.0.x * Sidekiq 7.1.x +* Sidekiq 7.2.x == Development diff --git a/lib/sidekiq/throttled/middleware.rb b/lib/sidekiq/throttled/middleware.rb index 54f777f9..c4d78927 100644 --- a/lib/sidekiq/throttled/middleware.rb +++ b/lib/sidekiq/throttled/middleware.rb @@ -9,7 +9,7 @@ module Throttled # # @private class Middleware - include Sidekiq::ServerMiddleware if Sidekiq::VERSION >= "6.5.0" + include Sidekiq::ServerMiddleware # Called within Sidekiq job processing def call(_worker, msg, _queue) diff --git a/sidekiq-throttled.gemspec b/sidekiq-throttled.gemspec index d67e5666..13159a5c 100644 --- a/sidekiq-throttled.gemspec +++ b/sidekiq-throttled.gemspec @@ -33,5 +33,5 @@ Gem::Specification.new do |spec| spec.required_ruby_version = ">= 3.0" spec.add_runtime_dependency "redis-prescription", "~> 2.2" - spec.add_runtime_dependency "sidekiq", ">= 6.5" + spec.add_runtime_dependency "sidekiq", ">= 7.0" end From d263ba96bc2d7fad6b670257d555f804442fcc20 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Fri, 17 Nov 2023 19:41:56 +0100 Subject: [PATCH 18/62] fix: Better patch BasicFetch This brings support of both Sidekiq::BasicFetch and Sidekiq::Pro::BasicFetch --- lib/sidekiq/throttled.rb | 32 ++++++++----------- lib/sidekiq/throttled/patches/basic_fetch.rb | 23 ++----------- .../throttled/patches/basic_fetch_spec.rb | 14 ++------ 3 files changed, 19 insertions(+), 50 deletions(-) diff --git a/lib/sidekiq/throttled.rb b/lib/sidekiq/throttled.rb index 8a549958..46a015fb 100644 --- a/lib/sidekiq/throttled.rb +++ b/lib/sidekiq/throttled.rb @@ -40,17 +40,17 @@ module Sidekiq # end # end module Throttled + @configuration = Configuration.new + class << self - # @return [Configuration] - def configuration - @configuration ||= Configuration.new - end + attr_reader :configuration - # Hooks throttler into sidekiq. - # - # @return [void] def setup! - Sidekiq::Throttled::Patches::BasicFetch.apply! + Sidekiq.configure_server do |config| + config.server_middleware do |chain| + chain.add Sidekiq::Throttled::Middleware + end + end end # Tells whenever job is throttled or not. @@ -58,11 +58,13 @@ def setup! # @param [String] message Job's JSON payload # @return [Boolean] def throttled?(message) - message = JSON.parse message - job = message.fetch("wrapped") { message.fetch("class") { return false } } - jid = message.fetch("jid") { return false } + message = Sidekiq.load_json(message) + job = message.fetch("wrapped") { message["class"] } + jid = message["jid"] - Registry.get job do |strategy| + return false unless job && jid + + Registry.get(job) do |strategy| return strategy.throttled?(jid, *message["args"]) end @@ -72,10 +74,4 @@ def throttled?(message) end end end - - configure_server do |config| - config.server_middleware do |chain| - chain.add Sidekiq::Throttled::Middleware - end - end end diff --git a/lib/sidekiq/throttled/patches/basic_fetch.rb b/lib/sidekiq/throttled/patches/basic_fetch.rb index c421520d..603db222 100644 --- a/lib/sidekiq/throttled/patches/basic_fetch.rb +++ b/lib/sidekiq/throttled/patches/basic_fetch.rb @@ -7,12 +7,6 @@ module Sidekiq module Throttled module Patches module BasicFetch - class << self - def apply! - Sidekiq::BasicFetch.prepend(self) unless Sidekiq::BasicFetch.include?(self) - end - end - # Retrieves job from redis. # # @return [Sidekiq::Throttled::UnitOfWork, nil] @@ -39,22 +33,9 @@ def retrieve_work def requeue_throttled(work) redis { |conn| conn.lpush(work.queue, work.job) } end - - # Returns list of queues to try to fetch jobs from. - # - # @note It may return an empty array. - # @param [Array] queues - # @return [Array] - def queues_cmd - queues = super - - # TODO: Refactor to be prepended as an integration mixin during configuration stage - # Or via configurable queues reducer - queues -= Sidekiq::Pauzer.paused_queues.map { |name| "queue:#{name}" } if defined?(Sidekiq::Pauzer) - - queues - end end end end end + +Sidekiq::BasicFetch.prepend(Sidekiq::Throttled::Patches::BasicFetch) diff --git a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb index 78c4de13..a814ecfc 100644 --- a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb +++ b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb @@ -11,19 +11,11 @@ def perform(*); end RSpec.describe Sidekiq::Throttled::Patches::BasicFetch do subject(:fetch) do - if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") - Sidekiq.instance_variable_set(:@config, Sidekiq::DEFAULTS.dup) - Sidekiq.queues = %w[default] - Sidekiq::BasicFetch.new(Sidekiq) - else - config = Sidekiq::Config.new - config.queues = %w[default] - Sidekiq::BasicFetch.new(config.default_capsule) - end + config = Sidekiq::Config.new + config.queues = %w[default] + Sidekiq::BasicFetch.new(config.default_capsule) end - before { described_class.apply! } - describe "#retrieve_work" do def enqueued_jobs(queue) Sidekiq.redis do |conn| From 0b42a9e0be0ef337fe2bd9e4850de8951788f742 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Sat, 18 Nov 2023 23:08:09 +0100 Subject: [PATCH 19/62] refactor: Remove configuration object Child jobs will inherit parent strategies without ability to opt-out. --- CHANGES.md | 14 +++++- lib/sidekiq/throttled.rb | 5 --- lib/sidekiq/throttled/configuration.rb | 50 --------------------- lib/sidekiq/throttled/registry.rb | 4 +- spec/lib/sidekiq/throttled/registry_spec.rb | 10 +---- 5 files changed, 15 insertions(+), 68 deletions(-) delete mode 100644 lib/sidekiq/throttled/configuration.rb diff --git a/CHANGES.md b/CHANGES.md index a4bd6ac5..0e51332d 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add Sidekiq-7.2 support + +### Changed + +- (BREAKING) Jobs inherit throttling strategy from their parent class, unless + explicitly overriden + ### Fixed - Correctly finalize throttled jobs when used with ActiveJob @@ -17,11 +26,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - (BREAKING) Drop Ruby-2.7.x support - (BREAKING) Drop Sidekiq-6.x.x support +- (BREAKING) Removed `Sidekiq::Throttled.configuration` ## [1.0.0.alpha.1] - 2023-06-08 -### Changes +### Changed - Upstream `Sidekiq::BasicFetch` is now infused with throttling directly, thus default fetch configuration should work on both Sidekiq and Sidekiq-Pro @@ -40,7 +50,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add Ruby 3.2 support -### Changes +### Changed - Switch README to Asciidoc format - Switch CHANGES to keepachangelog.com format diff --git a/lib/sidekiq/throttled.rb b/lib/sidekiq/throttled.rb index 46a015fb..5bac71d8 100644 --- a/lib/sidekiq/throttled.rb +++ b/lib/sidekiq/throttled.rb @@ -3,7 +3,6 @@ require "sidekiq" require_relative "./throttled/version" -require_relative "./throttled/configuration" require_relative "./throttled/patches/basic_fetch" require_relative "./throttled/registry" require_relative "./throttled/job" @@ -40,11 +39,7 @@ module Sidekiq # end # end module Throttled - @configuration = Configuration.new - class << self - attr_reader :configuration - def setup! Sidekiq.configure_server do |config| config.server_middleware do |chain| diff --git a/lib/sidekiq/throttled/configuration.rb b/lib/sidekiq/throttled/configuration.rb deleted file mode 100644 index 70c69679..00000000 --- a/lib/sidekiq/throttled/configuration.rb +++ /dev/null @@ -1,50 +0,0 @@ -# frozen_string_literal: true - -module Sidekiq - module Throttled - # Configuration holder. - class Configuration - # Class constructor. - def initialize - reset! - end - - # Reset configuration to defaults. - # - # @return [self] - def reset! - @inherit_strategies = false - - self - end - - # Instructs throttler to lookup strategies in parent classes, if there's - # no own strategy: - # - # class FooJob - # include Sidekiq::Job - # include Sidekiq::Throttled::Job - # - # sidekiq_throttle :concurrency => { :limit => 42 } - # end - # - # class BarJob < FooJob - # end - # - # By default in the example above, `Bar` won't have throttling options. - # Set this flag to `true` to enable this lookup in initializer, after - # that `Bar` will use `Foo` throttling bucket. - def inherit_strategies=(value) - @inherit_strategies = value ? true : false - end - - # Whenever throttled workers should inherit parent's strategies or not. - # Default: `false`. - # - # @return [Boolean] - def inherit_strategies? - @inherit_strategies - end - end - end -end diff --git a/lib/sidekiq/throttled/registry.rb b/lib/sidekiq/throttled/registry.rb index 1d46f351..b8e413a5 100644 --- a/lib/sidekiq/throttled/registry.rb +++ b/lib/sidekiq/throttled/registry.rb @@ -102,8 +102,6 @@ def find(name) # @param name [Class, #to_s] # @return [Strategy, nil] def find_by_class(name) - return unless Throttled.configuration.inherit_strategies? - const = name.is_a?(Class) ? name : Object.const_get(name) return unless const.is_a?(Class) @@ -112,6 +110,8 @@ def find_by_class(name) return strategy if strategy end + nil + rescue NameError nil end end diff --git a/spec/lib/sidekiq/throttled/registry_spec.rb b/spec/lib/sidekiq/throttled/registry_spec.rb index 7155742f..f7231585 100644 --- a/spec/lib/sidekiq/throttled/registry_spec.rb +++ b/spec/lib/sidekiq/throttled/registry_spec.rb @@ -76,15 +76,7 @@ def stub_class(name, *parent, &block) before { described_class.add(parent_class.name, **threshold) } - it { is_expected.to be_nil } - - context "when configuration has inherit strategy turned on" do - before { Sidekiq::Throttled.configuration.inherit_strategies = true } - - after { Sidekiq::Throttled.configuration.reset! } - - it { is_expected.to be described_class.get("Parent") } - end + it { is_expected.to be described_class.get("Parent") } end end From 934bdd97d013111717d4e0a4c8a4cd1ca44e6ec8 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Sat, 18 Nov 2023 04:08:20 +0100 Subject: [PATCH 20/62] feat!: Revive queues cooldown manager Brings back feature that was removed in: e5ac58594da7e5631c6c9bf3a076d32a82bc861e See README.adoc for possible configuration Resolve: https://github.com/ixti/sidekiq-throttled/pull/160 Resolve: https://github.com/ixti/sidekiq-throttled/issues/157 Co-authored-by: Alexandr Elhovenko Signed-off-by: Alexey Zapparov --- README.adoc | 19 +++ lib/sidekiq/throttled.rb | 36 +++++- lib/sidekiq/throttled/config.rb | 44 +++++++ lib/sidekiq/throttled/cooldown.rb | 55 ++++++++ lib/sidekiq/throttled/expirable_set.rb | 70 ++++++++++ lib/sidekiq/throttled/job.rb | 4 +- lib/sidekiq/throttled/patches/basic_fetch.rb | 12 ++ rubocop/rspec.yml | 4 + sidekiq-throttled.gemspec | 1 + spec/lib/sidekiq/throttled/config_spec.rb | 58 +++++++++ spec/lib/sidekiq/throttled/cooldown_spec.rb | 83 ++++++++++++ .../sidekiq/throttled/expirable_set_spec.rb | 64 +++++++++ .../throttled/patches/basic_fetch_spec.rb | 121 +++++++++++------- spec/spec_helper.rb | 8 ++ 14 files changed, 531 insertions(+), 48 deletions(-) create mode 100644 lib/sidekiq/throttled/config.rb create mode 100644 lib/sidekiq/throttled/cooldown.rb create mode 100644 lib/sidekiq/throttled/expirable_set.rb create mode 100644 spec/lib/sidekiq/throttled/config_spec.rb create mode 100644 spec/lib/sidekiq/throttled/cooldown_spec.rb create mode 100644 spec/lib/sidekiq/throttled/expirable_set_spec.rb diff --git a/README.adoc b/README.adoc index e704c168..0250dbe3 100644 --- a/README.adoc +++ b/README.adoc @@ -89,6 +89,25 @@ end ---- +=== Configuration + +[source,ruby] +---- +Sidekiq::Throttled.configure do |config| + # Period in seconds to exclude queue from polling in case it returned + # {config.cooldown_threshold} amount of throttled jobs in a row. Set + # this value to `nil` to disable cooldown manager completely. + # Default: 2.0 + config.cooldown_period = 2.0 + + # Exclude queue from polling after it returned given amount of throttled + # jobs in a row. + # Default: 1 (cooldown after first throttled job) + config.cooldown_threshold = 1 +end +---- + + === Observer You can specify an observer that will be called on throttling. To do so pass an diff --git a/lib/sidekiq/throttled.rb b/lib/sidekiq/throttled.rb index 5bac71d8..e3e1c76d 100644 --- a/lib/sidekiq/throttled.rb +++ b/lib/sidekiq/throttled.rb @@ -2,11 +2,13 @@ require "sidekiq" -require_relative "./throttled/version" -require_relative "./throttled/patches/basic_fetch" -require_relative "./throttled/registry" +require_relative "./throttled/config" +require_relative "./throttled/cooldown" require_relative "./throttled/job" require_relative "./throttled/middleware" +require_relative "./throttled/patches/basic_fetch" +require_relative "./throttled/registry" +require_relative "./throttled/version" require_relative "./throttled/worker" # @see https://github.com/mperham/sidekiq/ @@ -39,7 +41,35 @@ module Sidekiq # end # end module Throttled + MUTEX = Mutex.new + private_constant :MUTEX + + @config = Config.new.freeze + @cooldown = Cooldown[@config] + class << self + # @api internal + # + # @return [Cooldown, nil] + attr_reader :cooldown + + # @example + # Sidekiq::Throttled.configure do |config| + # config.cooldown_period = nil # Disable queues cooldown manager + # end + # + # @yieldparam config [Config] + def configure + MUTEX.synchronize do + config = @config.dup + + yield config + + @config = config.freeze + @cooldown = Cooldown[@config] + end + end + def setup! Sidekiq.configure_server do |config| config.server_middleware do |chain| diff --git a/lib/sidekiq/throttled/config.rb b/lib/sidekiq/throttled/config.rb new file mode 100644 index 00000000..d75a1195 --- /dev/null +++ b/lib/sidekiq/throttled/config.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module Sidekiq + module Throttled + # Configuration object. + class Config + # Period in seconds to exclude queue from polling in case it returned + # {#cooldown_threshold} amount of throttled jobs in a row. + # + # Set this to `nil` to disable cooldown completely. + # + # @return [Float, nil] + attr_reader :cooldown_period + + # Amount of throttled jobs returned from the queue subsequently after + # which queue will be excluded from polling for the durations of + # {#cooldown_period}. + # + # @return [Integer] + attr_reader :cooldown_threshold + + def initialize + @cooldown_period = 2.0 + @cooldown_threshold = 1 + end + + # @!attribute [w] cooldown_period + def cooldown_period=(value) + raise TypeError, "unexpected type #{value.class}" unless value.nil? || value.is_a?(Float) + raise ArgumentError, "period must be positive" unless value.nil? || value.positive? + + @cooldown_period = value + end + + # @!attribute [w] cooldown_threshold + def cooldown_threshold=(value) + raise TypeError, "unexpected type #{value.class}" unless value.is_a?(Integer) + raise ArgumentError, "threshold must be positive" unless value.positive? + + @cooldown_threshold = value + end + end + end +end diff --git a/lib/sidekiq/throttled/cooldown.rb b/lib/sidekiq/throttled/cooldown.rb new file mode 100644 index 00000000..11f38bda --- /dev/null +++ b/lib/sidekiq/throttled/cooldown.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require "concurrent" + +require_relative "./expirable_set" + +module Sidekiq + module Throttled + # @api internal + # + # Queues cooldown manager. Tracks list of queues that should be temporarily + # (for the duration of {Config#cooldown_period}) excluded from polling. + class Cooldown + class << self + # Returns new {Cooldown} instance if {Config#cooldown_period} is not `nil`. + # + # @param config [Config] + # @return [Cooldown, nil] + def [](config) + new(config) if config.cooldown_period + end + end + + # @param config [Config] + def initialize(config) + @queues = ExpirableSet.new(config.cooldown_period) + @threshold = config.cooldown_threshold + @tracker = Concurrent::Map.new + end + + # Notify that given queue returned job that was throttled. + # + # @param queue [String] + # @return [void] + def notify_throttled(queue) + @queues.add(queue) if @threshold <= @tracker.merge_pair(queue, 1, &:succ) + end + + # Notify that given queue returned job that was not throttled. + # + # @param queue [String] + # @return [void] + def notify_admitted(queue) + @tracker.delete(queue) + end + + # List of queues that should not be polled + # + # @return [Array] + def queues + @queues.to_a + end + end + end +end diff --git a/lib/sidekiq/throttled/expirable_set.rb b/lib/sidekiq/throttled/expirable_set.rb new file mode 100644 index 00000000..4242bf0d --- /dev/null +++ b/lib/sidekiq/throttled/expirable_set.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require "concurrent" + +module Sidekiq + module Throttled + # @api internal + # + # Set of elements with expirations. + # + # @example + # set = ExpirableSet.new(10.0) + # set.add("a") + # sleep(5) + # set.add("b") + # set.to_a # => ["a", "b"] + # sleep(5) + # set.to_a # => ["b"] + class ExpirableSet + include Enumerable + + # @param ttl [Float] expiration is seconds + # @raise [ArgumentError] if `ttl` is not positive Float + def initialize(ttl) + raise ArgumentError, "ttl must be positive Float" unless ttl.is_a?(Float) && ttl.positive? + + @elements = Concurrent::Map.new + @ttl = ttl + end + + # @param element [Object] + # @return [ExpirableSet] self + def add(element) + # cleanup expired elements to avoid mem-leak + horizon = now + expired = @elements.each_pair.select { |(_, sunset)| expired?(sunset, horizon) } + expired.each { |pair| @elements.delete_pair(*pair) } + + # add new element + @elements[element] = now + @ttl + + self + end + + # @yield [Object] Gives each live (not expired) element to the block + def each + return to_enum __method__ unless block_given? + + horizon = now + + @elements.each_pair do |element, sunset| + yield element unless expired?(sunset, horizon) + end + + self + end + + private + + # @return [Float] + def now + ::Process.clock_gettime(::Process::CLOCK_MONOTONIC) + end + + def expired?(sunset, horizon) + sunset <= horizon + end + end + end +end diff --git a/lib/sidekiq/throttled/job.rb b/lib/sidekiq/throttled/job.rb index d7401333..dca50d72 100644 --- a/lib/sidekiq/throttled/job.rb +++ b/lib/sidekiq/throttled/job.rb @@ -29,8 +29,8 @@ module Job # in order to make API inline with `include Sidekiq::Job`. # # @private - def self.included(worker) - worker.send(:extend, ClassMethods) + def self.included(base) + base.extend(ClassMethods) end # Helper methods added to the singleton class of destination diff --git a/lib/sidekiq/throttled/patches/basic_fetch.rb b/lib/sidekiq/throttled/patches/basic_fetch.rb index 603db222..0f933dc8 100644 --- a/lib/sidekiq/throttled/patches/basic_fetch.rb +++ b/lib/sidekiq/throttled/patches/basic_fetch.rb @@ -14,10 +14,13 @@ def retrieve_work work = super if work && Throttled.throttled?(work.job) + Throttled.cooldown&.notify_throttled(work.queue) requeue_throttled(work) return nil end + Throttled.cooldown&.notify_admitted(work.queue) if work + work end @@ -33,6 +36,15 @@ def retrieve_work def requeue_throttled(work) redis { |conn| conn.lpush(work.queue, work.job) } end + + # Returns list of queues to try to fetch jobs from. + # + # @note It may return an empty array. + # @param [Array] queues + # @return [Array] + def queues_cmd + super - (Throttled.cooldown&.queues || []) + end end end end diff --git a/rubocop/rspec.yml b/rubocop/rspec.yml index c129a663..8ca037ed 100644 --- a/rubocop/rspec.yml +++ b/rubocop/rspec.yml @@ -2,6 +2,10 @@ RSpec/ExampleLength: Enabled: true Max: 10 +RSpec/ExpectChange: + Enabled: true + EnforcedStyle: block + RSpec/MultipleExpectations: Enabled: false diff --git a/sidekiq-throttled.gemspec b/sidekiq-throttled.gemspec index 13159a5c..467217f3 100644 --- a/sidekiq-throttled.gemspec +++ b/sidekiq-throttled.gemspec @@ -32,6 +32,7 @@ Gem::Specification.new do |spec| spec.required_ruby_version = ">= 3.0" + spec.add_runtime_dependency "concurrent-ruby", ">= 1.2.0" spec.add_runtime_dependency "redis-prescription", "~> 2.2" spec.add_runtime_dependency "sidekiq", ">= 7.0" end diff --git a/spec/lib/sidekiq/throttled/config_spec.rb b/spec/lib/sidekiq/throttled/config_spec.rb new file mode 100644 index 00000000..6503f5f7 --- /dev/null +++ b/spec/lib/sidekiq/throttled/config_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require "sidekiq/throttled/config" + +RSpec.describe Sidekiq::Throttled::Config do + subject(:config) { described_class.new } + + describe "#cooldown_period" do + subject { config.cooldown_period } + + it { is_expected.to eq 2.0 } + end + + describe "#cooldown_period=" do + it "updates #cooldown_period" do + expect { config.cooldown_period = 42.0 } + .to change { config.cooldown_period }.to(42.0) + end + + it "allows setting value to `nil`" do + expect { config.cooldown_period = nil } + .to change { config.cooldown_period }.to(nil) + end + + it "fails if given value is neither `NilClass` nor `Float`" do + expect { config.cooldown_period = 42 } + .to raise_error(TypeError, %r{unexpected type}) + end + + it "fails if given value is not positive" do + expect { config.cooldown_period = 0.0 } + .to raise_error(ArgumentError, %r{must be positive}) + end + end + + describe "#cooldown_threshold" do + subject { config.cooldown_threshold } + + it { is_expected.to eq 1 } + end + + describe "#cooldown_threshold=" do + it "updates #cooldown_threshold" do + expect { config.cooldown_threshold = 42 } + .to change { config.cooldown_threshold }.to(42) + end + + it "fails if given value is not `Integer`" do + expect { config.cooldown_threshold = 42.0 } + .to raise_error(TypeError, %r{unexpected type}) + end + + it "fails if given value is not positive" do + expect { config.cooldown_threshold = 0 } + .to raise_error(ArgumentError, %r{must be positive}) + end + end +end diff --git a/spec/lib/sidekiq/throttled/cooldown_spec.rb b/spec/lib/sidekiq/throttled/cooldown_spec.rb new file mode 100644 index 00000000..9472259b --- /dev/null +++ b/spec/lib/sidekiq/throttled/cooldown_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require "sidekiq/throttled/cooldown" + +RSpec.describe Sidekiq::Throttled::Cooldown do + subject(:cooldown) { described_class.new(config) } + + let(:config) { Sidekiq::Throttled::Config.new } + + describe ".[]" do + subject { described_class[config] } + + it { is_expected.to be_an_instance_of described_class } + + context "when `cooldown_period` is nil" do + before { config.cooldown_period = nil } + + it { is_expected.to be_nil } + end + end + + describe "#notify_throttled" do + before do + config.cooldown_threshold = 5 + + (config.cooldown_threshold - 1).times do + cooldown.notify_throttled("queue:the_longest_line") + end + end + + it "marks queue for exclusion once threshold is met" do + cooldown.notify_throttled("queue:the_longest_line") + + expect(cooldown.queues).to contain_exactly("queue:the_longest_line") + end + end + + describe "#notify_admitted" do + before do + config.cooldown_threshold = 5 + + (config.cooldown_threshold - 1).times do + cooldown.notify_throttled("queue:at_the_end_of") + cooldown.notify_throttled("queue:the_longest_line") + end + end + + it "resets threshold counter" do + cooldown.notify_admitted("queue:at_the_end_of") + + cooldown.notify_throttled("queue:at_the_end_of") + cooldown.notify_throttled("queue:the_longest_line") + + expect(cooldown.queues).to contain_exactly("queue:the_longest_line") + end + end + + describe "#queues" do + before do + config.cooldown_period = 1.0 + config.cooldown_threshold = 1 + end + + it "keeps queue in the exclusion list for the duration of cooldown_period" do + monotonic_time = 0.0 + + allow(Process).to receive(:clock_gettime).with(Process::CLOCK_MONOTONIC) { monotonic_time } + + cooldown.notify_throttled("queue:at_the_end_of") + + monotonic_time += 0.9 + cooldown.notify_throttled("queue:the_longest_line") + + expect(cooldown.queues).to contain_exactly("queue:at_the_end_of", "queue:the_longest_line") + + monotonic_time += 0.1 + expect(cooldown.queues).to contain_exactly("queue:the_longest_line") + + monotonic_time += 1.0 + expect(cooldown.queues).to be_empty + end + end +end diff --git a/spec/lib/sidekiq/throttled/expirable_set_spec.rb b/spec/lib/sidekiq/throttled/expirable_set_spec.rb new file mode 100644 index 00000000..465c7ba9 --- /dev/null +++ b/spec/lib/sidekiq/throttled/expirable_set_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require "sidekiq/throttled/expirable_set" + +RSpec.describe Sidekiq::Throttled::ExpirableSet do + subject(:expirable_set) { described_class.new(2.0) } + + it { is_expected.to be_an Enumerable } + + describe ".new" do + it "raises ArgumentError if given TTL is not Float" do + expect { described_class.new(42) }.to raise_error(ArgumentError) + end + + it "raises ArgumentError if given TTL is not positive" do + expect { described_class.new(0.0) }.to raise_error(ArgumentError) + end + end + + describe "#add" do + it "returns self" do + expect(expirable_set.add("a")).to be expirable_set + end + + it "adds uniq elements to the set" do + expirable_set.add("a").add("b").add("b").add("a") + + expect(expirable_set).to contain_exactly("a", "b") + end + end + + describe "#each" do + subject { expirable_set.each } + + before do + monotonic_time = 0.0 + + allow(Process).to receive(:clock_gettime).with(Process::CLOCK_MONOTONIC) { monotonic_time } + + expirable_set.add("lorem") + expirable_set.add("ipsum") + + monotonic_time += 1 + + expirable_set.add("ipsum") + + monotonic_time += 1 + + expirable_set.add("dolor") + end + + it { is_expected.to be_an(Enumerator) } + it { is_expected.to contain_exactly("ipsum", "dolor") } + + context "with block given" do + it "yields each paused queue and returns self" do + yielded_elements = [] + + expect(expirable_set.each { |element| yielded_elements << element }).to be expirable_set + expect(yielded_elements).to contain_exactly("ipsum", "dolor") + end + end + end +end diff --git a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb index a814ecfc..84b10fae 100644 --- a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb +++ b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb @@ -2,75 +2,110 @@ require "sidekiq/throttled/patches/basic_fetch" -class ThrottledTestJob - include Sidekiq::Job - include Sidekiq::Throttled::Job +RSpec.describe Sidekiq::Throttled::Patches::BasicFetch do + def stub_job_class(name, &block) + klass = stub_const(name, Class.new) - def perform(*); end -end + klass.include(Sidekiq::Job) + klass.include(Sidekiq::Throttled::Job) -RSpec.describe Sidekiq::Throttled::Patches::BasicFetch do - subject(:fetch) do - config = Sidekiq::Config.new - config.queues = %w[default] - Sidekiq::BasicFetch.new(config.default_capsule) + klass.instance_exec do + def perform(*); end + end + + klass.instance_exec(&block) if block end - describe "#retrieve_work" do - def enqueued_jobs(queue) - Sidekiq.redis do |conn| - conn.lrange("queue:#{queue}", 0, -1).map do |job| - JSON.parse(job).then do |payload| - [payload["class"], payload["args"]] - end + def enqueued_jobs(queue) + Sidekiq.redis do |conn| + conn.lrange("queue:#{queue}", 0, -1).map do |job| + JSON.parse(job).then do |payload| + [payload["class"], *payload["args"]] end end end + end - before do - # Sidekiq is FIFO queue, with head on right side of the list, - # meaning jobs below will be stored in 3, 2, 1 order. - ThrottledTestJob.perform_bulk([[1], [2], [3]]) - end + let(:fetch) do + config = Sidekiq::Config.new + config.queues = %w[default critical] + Sidekiq::BasicFetch.new(config.default_capsule) + end + + before do + Sidekiq::Throttled.configure { |config| config.cooldown_period = nil } + + stub_job_class("TestJob") + stub_job_class("AnotherTestJob") { sidekiq_options(queue: :critical) } + + allow(Process).to receive(:clock_gettime).with(Process::CLOCK_MONOTONIC).and_return(0.0) + + # Sidekiq is FIFO queue, with head on right side of the list, + # meaning jobs below will be stored in 3, 2, 1 order. + TestJob.perform_async(1) + TestJob.perform_async(2) + TestJob.perform_async(3) + AnotherTestJob.perform_async(4) + end + describe "#retrieve_work" do context "when job is not throttled" do it "returns unit of work" do - expect(fetch.retrieve_work).to be_an_instance_of(Sidekiq::BasicFetch::UnitOfWork) + expect(Array.new(4) { fetch.retrieve_work }).to all be_an_instance_of(Sidekiq::BasicFetch::UnitOfWork) end end - context "when job was throttled due to concurrency" do - before do - ThrottledTestJob.sidekiq_throttle(concurrency: { limit: 1 }) + shared_examples "requeues throttled job" do + it "returns nothing" do fetch.retrieve_work - end - it "returns nothing" do expect(fetch.retrieve_work).to be_nil end it "pushes job back to the head of the queue" do + fetch.retrieve_work + expect { fetch.retrieve_work } - .to change { enqueued_jobs("default") } - .to eq([["ThrottledTestJob", [2]], ["ThrottledTestJob", [3]]]) + .to change { enqueued_jobs("default") }.to([["TestJob", 2], ["TestJob", 3]]) + .and(keep_unchanged { enqueued_jobs("critical") }) end - end - context "when job was throttled due to threshold" do - before do - ThrottledTestJob.sidekiq_throttle(threshold: { limit: 1, period: 60 }) - fetch.retrieve_work - end + context "when queue cooldown kicks in" do + before do + Sidekiq::Throttled.configure do |config| + config.cooldown_period = 2.0 + config.cooldown_threshold = 1 + end - it "returns nothing" do - expect(fetch.retrieve_work).to be_nil - end + fetch.retrieve_work + end - it "pushes job back to the head of the queue" do - expect { fetch.retrieve_work } - .to change { enqueued_jobs("default") } - .to eq([["ThrottledTestJob", [2]], ["ThrottledTestJob", [3]]]) + it "updates cooldown queues" do + expect { fetch.retrieve_work } + .to change { enqueued_jobs("default") }.to([["TestJob", 2], ["TestJob", 3]]) + .and(change { Sidekiq::Throttled.cooldown.queues }.to(["queue:default"])) + end + + it "excludes the queue from polling" do + fetch.retrieve_work + + expect { fetch.retrieve_work } + .to change { enqueued_jobs("critical") }.to([]) + .and(keep_unchanged { enqueued_jobs("default") }) + end end end + + context "when job was throttled due to concurrency" do + before { TestJob.sidekiq_throttle(concurrency: { limit: 1 }) } + + include_examples "requeues throttled job" + end + + context "when job was throttled due to threshold" do + before { TestJob.sidekiq_throttle(threshold: { limit: 1, period: 60 }) } + + include_examples "requeues throttled job" + end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index ce733462..80c1093f 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -15,6 +15,14 @@ @strategies.clear @aliases.clear end + + # Reset config + Sidekiq::Throttled.configure do |throttled_config| + defaults = Sidekiq::Throttled::Config.new + + throttled_config.cooldown_period = defaults.cooldown_period + throttled_config.cooldown_threshold = defaults.cooldown_threshold + end end # rspec-expectations config goes here. You can use an alternate From b2949ccdc63819366eb0b413d2783491ac8f6aa8 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Mon, 20 Nov 2023 03:00:38 +0100 Subject: [PATCH 21/62] cleanup: Use Hash#[] instead of fetch --- lib/sidekiq/throttled/middleware.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/sidekiq/throttled/middleware.rb b/lib/sidekiq/throttled/middleware.rb index c4d78927..6a482c3c 100644 --- a/lib/sidekiq/throttled/middleware.rb +++ b/lib/sidekiq/throttled/middleware.rb @@ -15,8 +15,8 @@ class Middleware def call(_worker, msg, _queue) yield ensure - job = msg.fetch("wrapped") { msg.fetch("class", false) } - jid = msg.fetch("jid", false) + job = msg.fetch("wrapped") { msg["class"] } + jid = msg["jid"] if job && jid Registry.get job do |strategy| From f6776b866801a8fcd626de319baa7798c2fe21ed Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Mon, 20 Nov 2023 03:04:20 +0100 Subject: [PATCH 22/62] Release v1.0.0 --- CHANGES.md | 7 ++++++- lib/sidekiq/throttled/version.rb | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 0e51332d..f70f69cb 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,9 +8,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [1.0.0] - 2023-11-20 + ### Added - Add Sidekiq-7.2 support +- Revive queues cooldown logic + [#163](https://github.com/ixti/sidekiq-throttled/pull/163) ### Changed @@ -66,6 +70,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Remove queue exclusion from fetcher pon throttled job -[unreleased]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0.alpha.1...main +[unreleased]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0...main +[1.0.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0.alpha.1...v1.0.0 [1.0.0.alpha.1]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0.alpha...v1.0.0.alpha.1 [1.0.0.alpha]: https://github.com/ixti/sidekiq-throttled/compare/v0.16.1...v1.0.0.alpha diff --git a/lib/sidekiq/throttled/version.rb b/lib/sidekiq/throttled/version.rb index 0069537a..042cae79 100644 --- a/lib/sidekiq/throttled/version.rb +++ b/lib/sidekiq/throttled/version.rb @@ -3,6 +3,6 @@ module Sidekiq module Throttled # Gem version - VERSION = "1.0.0.alpha.1" + VERSION = "1.0.0" end end From 7a4db28d427cc6955c50d9088dc802f2c7d7690b Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Mon, 20 Nov 2023 17:25:01 +0100 Subject: [PATCH 23/62] Release v1.0.1 Adds back Sidekiq-6.5.x support Resolve: https://github.com/ixti/sidekiq-throttled/issues/165 Signed-off-by: Alexey Zapparov --- Appraisals | 6 ++++++ CHANGES.md | 10 +++++++++- README.adoc | 1 + lib/sidekiq/throttled/version.rb | 2 +- sidekiq-throttled.gemspec | 2 +- .../sidekiq/throttled/patches/basic_fetch_spec.rb | 12 +++++++++--- 6 files changed, 27 insertions(+), 6 deletions(-) diff --git a/Appraisals b/Appraisals index 7e7c8815..c62504a1 100644 --- a/Appraisals +++ b/Appraisals @@ -1,5 +1,11 @@ # frozen_string_literal: true +appraise "sidekiq-6.5.x" do + group :test do + gem "sidekiq", "~> 6.5.0" + end +end + appraise "sidekiq-7.0.x" do group :test do gem "sidekiq", "~> 7.0.0" diff --git a/CHANGES.md b/CHANGES.md index f70f69cb..17a8b31c 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [1.0.1] - 2023-11-20 + +### Added + +- Bring back Sidekiq-6.5 support + + ## [1.0.0] - 2023-11-20 ### Added @@ -70,7 +77,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Remove queue exclusion from fetcher pon throttled job -[unreleased]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0...main +[unreleased]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.1...main +[1.0.1]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0...v1.0.1 [1.0.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0.alpha.1...v1.0.0 [1.0.0.alpha.1]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0.alpha...v1.0.0.alpha.1 [1.0.0.alpha]: https://github.com/ixti/sidekiq-throttled/compare/v0.16.1...v1.0.0.alpha diff --git a/README.adoc b/README.adoc index 0250dbe3..fa114e5d 100644 --- a/README.adoc +++ b/README.adoc @@ -273,6 +273,7 @@ dropped. This library aims to support and work with following Sidekiq versions: +* Sidekiq 6.5.x * Sidekiq 7.0.x * Sidekiq 7.1.x * Sidekiq 7.2.x diff --git a/lib/sidekiq/throttled/version.rb b/lib/sidekiq/throttled/version.rb index 042cae79..03e310cc 100644 --- a/lib/sidekiq/throttled/version.rb +++ b/lib/sidekiq/throttled/version.rb @@ -3,6 +3,6 @@ module Sidekiq module Throttled # Gem version - VERSION = "1.0.0" + VERSION = "1.0.1" end end diff --git a/sidekiq-throttled.gemspec b/sidekiq-throttled.gemspec index 467217f3..d170d539 100644 --- a/sidekiq-throttled.gemspec +++ b/sidekiq-throttled.gemspec @@ -34,5 +34,5 @@ Gem::Specification.new do |spec| spec.add_runtime_dependency "concurrent-ruby", ">= 1.2.0" spec.add_runtime_dependency "redis-prescription", "~> 2.2" - spec.add_runtime_dependency "sidekiq", ">= 7.0" + spec.add_runtime_dependency "sidekiq", ">= 6.5" end diff --git a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb index 84b10fae..680aa8e2 100644 --- a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb +++ b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb @@ -27,9 +27,15 @@ def enqueued_jobs(queue) end let(:fetch) do - config = Sidekiq::Config.new - config.queues = %w[default critical] - Sidekiq::BasicFetch.new(config.default_capsule) + if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") + Sidekiq.instance_variable_set(:@config, Sidekiq::DEFAULTS.dup) + Sidekiq.queues = %w[default critical] + Sidekiq::BasicFetch.new(Sidekiq) + else + config = Sidekiq::Config.new + config.queues = %w[default critical] + Sidekiq::BasicFetch.new(config.default_capsule) + end end before do From bd63f83a254f5edf95f6cdad062a949ebb577fea Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Tue, 21 Nov 2023 07:05:38 +0100 Subject: [PATCH 24/62] refactor!: Deprecate setup! * 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 --- CHANGES.md | 10 ++++++ LICENSE => LICENSE.txt | 0 README.adoc | 25 ++++++++++++--- lib/sidekiq/throttled.rb | 29 +++++++++++------ lib/sidekiq/throttled/middleware.rb | 29 ----------------- lib/sidekiq/throttled/middlewares/server.rb | 28 ++++++++++++++++ sidekiq-throttled.gemspec | 2 +- .../server_spec.rb} | 10 +++--- spec/lib/sidekiq/throttled_spec.rb | 32 +++++++++---------- 9 files changed, 100 insertions(+), 65 deletions(-) rename LICENSE => LICENSE.txt (100%) delete mode 100644 lib/sidekiq/throttled/middleware.rb create mode 100644 lib/sidekiq/throttled/middlewares/server.rb rename spec/lib/sidekiq/throttled/{middleware_spec.rb => middlewares/server_spec.rb} (90%) diff --git a/CHANGES.md b/CHANGES.md index 17a8b31c..7ea0f027 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Renamed `Sidekiq::Throttled::Middleware` to `Sidekiq::Throttled::Middlewares::Server` + +### Deprecated + +- `Sidekiq::Throttled.setup!` is now deprecated. If you need to change order of + the middleware, please manipulate middlewares chains directly. + + ## [1.0.1] - 2023-11-20 ### Added diff --git a/LICENSE b/LICENSE.txt similarity index 100% rename from LICENSE rename to LICENSE.txt diff --git a/README.adoc b/README.adoc index fa114e5d..bbde7cf6 100644 --- a/README.adoc +++ b/README.adoc @@ -44,12 +44,8 @@ you are using Rails): [source,ruby] ---- require "sidekiq/throttled" -Sidekiq::Throttled.setup! ---- -Load order can be an issue if you are using other Sidekiq plugins and/or middleware. -To prevent any problems, add the `.setup!` call to the bottom of your init file. - Once you've done that you can include `Sidekiq::Throttled::Job` to your job classes and configure throttling: @@ -89,6 +85,27 @@ end ---- +=== Middleware(s) + +`Sidekiq::Throttled` relies on following bundled middlewares: + +* `Sidekiq::Throttled::Middlewares::Server` + +The middleware is automatically injected when you require `sidekiq/throttled`. +In rare cases this might be an issue. You can change to order manually: + +[source,ruby] +---- +Sidekiq.configure_server do |config| + # ... + + config.server_middleware do |chain| + chain.remove(Sidekiq::Throttled::Middlewares::Server) + chain.add(Sidekiq::Throttled::Middlewares::Server) + end +end +---- + === Configuration [source,ruby] diff --git a/lib/sidekiq/throttled.rb b/lib/sidekiq/throttled.rb index e3e1c76d..86458f38 100644 --- a/lib/sidekiq/throttled.rb +++ b/lib/sidekiq/throttled.rb @@ -5,7 +5,7 @@ require_relative "./throttled/config" require_relative "./throttled/cooldown" require_relative "./throttled/job" -require_relative "./throttled/middleware" +require_relative "./throttled/middlewares/server" require_relative "./throttled/patches/basic_fetch" require_relative "./throttled/registry" require_relative "./throttled/version" @@ -18,7 +18,6 @@ module Sidekiq # Just add somewhere in your bootstrap: # # require "sidekiq/throttled" - # Sidekiq::Throttled.setup! # # Once you've done that you can include {Sidekiq::Throttled::Job} to your # job classes and configure throttling: @@ -70,14 +69,6 @@ def configure end end - def setup! - Sidekiq.configure_server do |config| - config.server_middleware do |chain| - chain.add Sidekiq::Throttled::Middleware - end - end - end - # Tells whenever job is throttled or not. # # @param [String] message Job's JSON payload @@ -97,6 +88,24 @@ def throttled?(message) rescue false end + + # @deprecated Will be removed in 2.0.0 + def setup! + warn "Sidekiq::Throttled.setup! was deprecated" + + Sidekiq.configure_server do |config| + config.server_middleware do |chain| + chain.remove(Sidekiq::Throttled::Middlewares::Server) + chain.add(Sidekiq::Throttled::Middlewares::Server) + end + end + end + end + end + + configure_server do |config| + config.server_middleware do |chain| + chain.add(Sidekiq::Throttled::Middlewares::Server) end end end diff --git a/lib/sidekiq/throttled/middleware.rb b/lib/sidekiq/throttled/middleware.rb deleted file mode 100644 index 6a482c3c..00000000 --- a/lib/sidekiq/throttled/middleware.rb +++ /dev/null @@ -1,29 +0,0 @@ -# frozen_string_literal: true - -# internal -require_relative "./registry" - -module Sidekiq - module Throttled - # Server middleware that notifies strategy that job was finished. - # - # @private - class Middleware - include Sidekiq::ServerMiddleware - - # Called within Sidekiq job processing - def call(_worker, msg, _queue) - yield - ensure - job = msg.fetch("wrapped") { msg["class"] } - jid = msg["jid"] - - if job && jid - Registry.get job do |strategy| - strategy.finalize!(jid, *msg["args"]) - end - end - end - end - end -end diff --git a/lib/sidekiq/throttled/middlewares/server.rb b/lib/sidekiq/throttled/middlewares/server.rb new file mode 100644 index 00000000..7ded3172 --- /dev/null +++ b/lib/sidekiq/throttled/middlewares/server.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +# internal +require_relative "../registry" + +module Sidekiq + module Throttled + module Middlewares + # Server middleware required for Sidekiq::Throttled functioning. + class Server + include Sidekiq::ServerMiddleware + + def call(_worker, msg, _queue) + yield + ensure + job = msg.fetch("wrapped") { msg["class"] } + jid = msg["jid"] + + if job && jid + Registry.get job do |strategy| + strategy.finalize!(jid, *msg["args"]) + end + end + end + end + end + end +end diff --git a/sidekiq-throttled.gemspec b/sidekiq-throttled.gemspec index d170d539..8b9a1122 100644 --- a/sidekiq-throttled.gemspec +++ b/sidekiq-throttled.gemspec @@ -19,7 +19,7 @@ Gem::Specification.new do |spec| spec.metadata["rubygems_mfa_required"] = "true" spec.files = Dir.chdir(__dir__) do - docs = %w[LICENSE README.adoc].freeze + docs = %w[LICENSE.txt README.adoc].freeze `git ls-files -z`.split("\x0").select do |f| f.start_with?("lib/") || docs.include?(f) diff --git a/spec/lib/sidekiq/throttled/middleware_spec.rb b/spec/lib/sidekiq/throttled/middlewares/server_spec.rb similarity index 90% rename from spec/lib/sidekiq/throttled/middleware_spec.rb rename to spec/lib/sidekiq/throttled/middlewares/server_spec.rb index cbf21687..cc6170fc 100644 --- a/spec/lib/sidekiq/throttled/middleware_spec.rb +++ b/spec/lib/sidekiq/throttled/middlewares/server_spec.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true -require "sidekiq/throttled/middleware" +require "sidekiq/throttled/middlewares/server" -RSpec.describe Sidekiq::Throttled::Middleware do +RSpec.describe Sidekiq::Throttled::Middlewares::Server do subject(:middleware) { described_class.new } describe "#call" do @@ -16,7 +16,7 @@ it "calls #finalize! of queue with jid of job being processed" do expect(strategy).to receive(:finalize!).with "bar" - middleware.call(double, payload, double) { |*| } + middleware.call(double, payload, double) { |*| :foobar } end it "returns yields control to the given block" do @@ -46,7 +46,7 @@ it "calls #finalize! of queue with jid of job being processed" do expect(strategy).to receive(:finalize!).with "bar" - middleware.call(double, payload, double) { |*| } + middleware.call(double, payload, double) { |*| :foobar } end it "returns yields control to the given block" do @@ -68,7 +68,7 @@ it "calls #finalize! of queue with jid of job being processed" do expect(strategy).to receive(:finalize!).with "bar" - middleware.call(double, payload, double) { |*| } + middleware.call(double, payload, double) { |*| :foobar } end it "returns yields control to the given block" do diff --git a/spec/lib/sidekiq/throttled_spec.rb b/spec/lib/sidekiq/throttled_spec.rb index 970122f8..7d0341d9 100644 --- a/spec/lib/sidekiq/throttled_spec.rb +++ b/spec/lib/sidekiq/throttled_spec.rb @@ -3,25 +3,25 @@ require "json" RSpec.describe Sidekiq::Throttled do - describe ".setup!" do - before do - require "sidekiq/processor" - allow(Sidekiq).to receive(:server?).and_return true - described_class.setup! + it "registers server middleware" do + require "sidekiq/processor" + allow(Sidekiq).to receive(:server?).and_return true + + if Sidekiq::VERSION >= "7.0" + expect(Sidekiq.default_configuration.server_middleware.exists?(Sidekiq::Throttled::Middlewares::Server)) + .to be true + else + expect(Sidekiq.server_middleware.exists?(Sidekiq::Throttled::Middlewares::Server)).to be true end + end - it "infuses Sidekiq::BasicFetch with our patches" do - expect(Sidekiq::BasicFetch).to include(Sidekiq::Throttled::Patches::BasicFetch) - end + it "infuses Sidekiq::BasicFetch with our patches" do + expect(Sidekiq::BasicFetch).to include(Sidekiq::Throttled::Patches::BasicFetch) + end - it "injects Sidekiq::Throttled::Middleware server middleware" do - if Sidekiq::VERSION >= "7.0" - expect(Sidekiq.default_configuration.server_middleware.exists?(Sidekiq::Throttled::Middleware)) - .to be true - else - expect(Sidekiq.server_middleware.exists?(Sidekiq::Throttled::Middleware)) - .to be true - end + describe ".setup!" do + it "shows deprecation warning" do + expect { described_class.setup! }.to output(%r{deprecated}).to_stderr end end From d382193d17884f46dade066ba77335c676580a5f Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Tue, 21 Nov 2023 18:10:16 +0100 Subject: [PATCH 25/62] Release v1.1.0 --- CHANGES.md | 5 ++++- lib/sidekiq/throttled/version.rb | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 7ea0f027..1d665ab5 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [1.1.0] - 2023-11-21 + ### Changed - Renamed `Sidekiq::Throttled::Middleware` to `Sidekiq::Throttled::Middlewares::Server` @@ -87,7 +89,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Remove queue exclusion from fetcher pon throttled job -[unreleased]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.1...main +[unreleased]: https://github.com/ixti/sidekiq-throttled/compare/v1.1.0...main +[1,1.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.1...v1.1.0 [1.0.1]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0...v1.0.1 [1.0.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0.alpha.1...v1.0.0 [1.0.0.alpha.1]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0.alpha...v1.0.0.alpha.1 diff --git a/lib/sidekiq/throttled/version.rb b/lib/sidekiq/throttled/version.rb index 03e310cc..f6306329 100644 --- a/lib/sidekiq/throttled/version.rb +++ b/lib/sidekiq/throttled/version.rb @@ -3,6 +3,6 @@ module Sidekiq module Throttled # Gem version - VERSION = "1.0.1" + VERSION = "1.1.0" end end From f4dc961a1ec6c403fc41f618abccb7cab81b5d36 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Tue, 21 Nov 2023 20:41:26 +0100 Subject: [PATCH 26/62] doc: Add link to Sidekiq midlewares chain API --- README.adoc | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/README.adoc b/README.adoc index bbde7cf6..8ac99991 100644 --- a/README.adoc +++ b/README.adoc @@ -15,7 +15,7 @@ NOTE: This is the 1.x *development* branch. For the 0.x *stable* branch, please see: https://github.com/ixti/sidekiq-throttled/tree/0-x-stable[0-x-stable] -Concurrency and threshold throttling for https://github.com/mperham/sidekiq[Sidekiq]. +Concurrency and threshold throttling for https://github.com/sidekiq/sidekiq[Sidekiq]. == Installation @@ -85,7 +85,26 @@ end ---- -=== Middleware(s) +=== Configuration + +[source,ruby] +---- +Sidekiq::Throttled.configure do |config| + # Period in seconds to exclude queue from polling in case it returned + # {config.cooldown_threshold} amount of throttled jobs in a row. Set + # this value to `nil` to disable cooldown manager completely. + # Default: 2.0 + config.cooldown_period = 2.0 + + # Exclude queue from polling after it returned given amount of throttled + # jobs in a row. + # Default: 1 (cooldown after first throttled job) + config.cooldown_threshold = 1 +end +---- + + +==== Middleware(s) `Sidekiq::Throttled` relies on following bundled middlewares: @@ -106,23 +125,7 @@ Sidekiq.configure_server do |config| end ---- -=== Configuration - -[source,ruby] ----- -Sidekiq::Throttled.configure do |config| - # Period in seconds to exclude queue from polling in case it returned - # {config.cooldown_threshold} amount of throttled jobs in a row. Set - # this value to `nil` to disable cooldown manager completely. - # Default: 2.0 - config.cooldown_period = 2.0 - - # Exclude queue from polling after it returned given amount of throttled - # jobs in a row. - # Default: 1 (cooldown after first throttled job) - config.cooldown_threshold = 1 -end ----- +See: https://github.com/sidekiq/sidekiq/blob/main/lib/sidekiq/middleware/chain.rb === Observer From 0d8d1a7a2e29c95128e76a4c5d3c031feaddf3b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20Mari=C3=A9?= Date: Wed, 22 Nov 2023 18:20:39 +0100 Subject: [PATCH 27/62] doc: Remove warning about 0.x being the stable version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1.x is stable now as per rubygems Signed-off-by: Dorian Marié --- README.adoc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/README.adoc b/README.adoc index 8ac99991..aa131ee1 100644 --- a/README.adoc +++ b/README.adoc @@ -12,12 +12,8 @@ {doc-link}[image:{doc-badge}[API Documentation]] **** -NOTE: This is the 1.x *development* branch. For the 0.x *stable* branch, please - see: https://github.com/ixti/sidekiq-throttled/tree/0-x-stable[0-x-stable] - Concurrency and threshold throttling for https://github.com/sidekiq/sidekiq[Sidekiq]. - == Installation Add this line to your application's Gemfile: From 378b66e8a48a60ceeb6cf339974316f1d87a8cc1 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Fri, 8 Dec 2023 23:24:59 +0100 Subject: [PATCH 28/62] doc: Add SensorTower endorsement --- README.adoc | 8 ++++++++ sensortower.svg | 1 + 2 files changed, 9 insertions(+) create mode 100644 sensortower.svg diff --git a/README.adoc b/README.adoc index aa131ee1..8f28415c 100644 --- a/README.adoc +++ b/README.adoc @@ -311,3 +311,11 @@ This library aims to support and work with following Sidekiq versions: * Send a pull request * If we like them we'll merge them * If we've accepted a patch, feel free to ask for commit access! + + +== Endorsement + +https://github.com/sensortower[image:sensortower.svg[SensorTower]] + +The initial work on the project was initiated to address the needs of +https://github.com/sensortower[SensorTower]. diff --git a/sensortower.svg b/sensortower.svg new file mode 100644 index 00000000..0ae9f6af --- /dev/null +++ b/sensortower.svg @@ -0,0 +1 @@ + \ No newline at end of file From de57ddd2eb8823ec35aa7fce068710c0172e9ac2 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Tue, 12 Dec 2023 19:43:52 +0100 Subject: [PATCH 29/62] ci: Add codecov --- .github/workflows/ci.yml | 26 +++++++++++++++----------- .rspec | 5 +---- .simplecov | 25 +++++++++++++++++++++++++ Gemfile | 3 +++ Rakefile | 29 +++++++++++++++++++++++++---- scripts/ci-rspec | 17 ----------------- spec/spec_helper.rb | 1 - spec/support/simplecov.rb | 13 ------------- 8 files changed, 69 insertions(+), 50 deletions(-) create mode 100644 .simplecov delete mode 100755 scripts/ci-rspec delete mode 100644 spec/support/simplecov.rb diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c1e19aba..861be1de 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,12 +8,7 @@ on: jobs: test: - name: "rspec (ruby:${{ matrix.ruby }}" - - strategy: - fail-fast: false - matrix: - ruby: [ "3.0", "3.1", "3.2" ] + name: "rspec (ruby:${{ matrix.ruby }})" runs-on: ubuntu-latest @@ -28,6 +23,11 @@ jobs: --health-timeout 5s --health-retries 5 + strategy: + fail-fast: false + matrix: + ruby: [ ruby-3.0, ruby-3.1, ruby-3.2 ] + steps: - uses: actions/checkout@v4 @@ -36,10 +36,14 @@ jobs: ruby-version: ${{ matrix.ruby }} bundler-cache: true - - name: RSpec - run: scripts/ci-rspec + - run: bundle exec rake test + + - name: Upload coverage reports to Codecov + uses: codecov/codecov-action@v3 + env: + CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} - rubocop: + lint: runs-on: ubuntu-latest steps: @@ -47,7 +51,7 @@ jobs: - uses: ruby/setup-ruby@v1 with: - ruby-version: "3.2" + ruby-version: ruby-3.2 bundler-cache: true - - run: bundle exec rubocop + - run: bundle exec rake lint diff --git a/.rspec b/.rspec index d3ec40e0..283edea7 100644 --- a/.rspec +++ b/.rspec @@ -1,5 +1,2 @@ ---backtrace ---color ---format=documentation ---order random +--require simplecov --require spec_helper diff --git a/.simplecov b/.simplecov new file mode 100644 index 00000000..193cb89e --- /dev/null +++ b/.simplecov @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +SimpleCov.start do + gemfile = File.basename(ENV.fetch("BUNDLE_GEMFILE", "Gemfile"), ".gemfile").strip + gemfile = nil if gemfile.empty? || gemfile.casecmp?("gems.rb") || gemfile.casecmp?("Gemfile") + + command_name ["#{RUBY_ENGINE}-#{RUBY_ENGINE_VERSION}", gemfile].compact.join("/") + + enable_coverage :branch + + if ENV["CI"] + require "simplecov-cobertura" + formatter SimpleCov::Formatter::CoberturaFormatter + else + formatter SimpleCov::Formatter::MultiFormatter.new([ + SimpleCov::Formatter::SimpleFormatter, + SimpleCov::Formatter::HTMLFormatter + ]) + end + + add_filter "/demo/" + add_filter "/gemfiles/" + add_filter "/spec/" + add_filter "/vendor/" +end diff --git a/Gemfile b/Gemfile index 0589cf19..36d44f05 100644 --- a/Gemfile +++ b/Gemfile @@ -15,7 +15,10 @@ group :test do gem "rack-test" gem "rspec" + gem "simplecov" + gem "simplecov-cobertura" + gem "timecop" gem "rubocop", require: false diff --git a/Rakefile b/Rakefile index ba007163..f8aa96de 100644 --- a/Rakefile +++ b/Rakefile @@ -1,9 +1,30 @@ # frozen_string_literal: true -require "appraisal" require "bundler/gem_tasks" -require "rspec/core/rake_task" -RSpec::Core::RakeTask.new +desc "Run tests" +task :test do + rm_rf "coverage" + rm_rf "gemfiles" -task default: ENV["APPRAISAL_INITIALIZED"] ? %i[spec] : %i[appraisal rubocop] + Bundler.with_unbundled_env do + sh "bundle exec appraisal generate" + + # XXX: `bundle exec appraisal install` fails on ruby-3.2 + Dir["gemfiles/*.gemfile"].each do |gemfile| + sh({ "BUNDLE_GEMFILE" => gemfile }, "bundle lock") + sh({ "BUNDLE_GEMFILE" => gemfile }, "bundle check") do |ok| + sh({ "BUNDLE_GEMFILE" => gemfile }, "bundle install") unless ok + end + end + + sh "bundle exec appraisal rspec --force-colour" + end +end + +desc "Lint codebase" +task :lint do + sh "bundle exec rubocop --color" +end + +task default: %i[test lint] diff --git a/scripts/ci-rspec b/scripts/ci-rspec deleted file mode 100755 index 1aedbca0..00000000 --- a/scripts/ci-rspec +++ /dev/null @@ -1,17 +0,0 @@ -#!/usr/bin/env bash - -set -Eeuxo pipefail - -rm -f ./Gemfile.lock -bundle install - -rm -f ./gemfiles/*.gemfile ./gemfiles/*.gemfile.lock -bundle exec appraisal generate - -# XXX: `bundle exec appraisal install` fails on CI with ruby-3.2 -for BUNDLE_GEMFILE in gemfiles/*.gemfile; do - export BUNDLE_GEMFILE - bundle check || bundle install -done - -bundle exec appraisal rspec diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 80c1093f..cb34b713 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true -require_relative "support/simplecov" if ENV["CI"] || ENV["COVERAGE"] require_relative "support/sidekiq" require_relative "support/timecop" diff --git a/spec/support/simplecov.rb b/spec/support/simplecov.rb deleted file mode 100644 index ca4f6f67..00000000 --- a/spec/support/simplecov.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -require "simplecov" - -SimpleCov.start do - command_name "BUNDLE_GEMFILE=#{ENV.fetch('BUNDLE_GEMFILE')}" - - enable_coverage :branch - - add_filter "/gemfiles/" - add_filter "/spec/" - add_filter "/vendor/" -end From 81d5a2b84324a02e2e9167d3fc8cad11292ef61a Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Tue, 12 Dec 2023 19:46:31 +0100 Subject: [PATCH 30/62] fix: Update rubocop config --- lib/sidekiq/throttled.rb | 2 +- lib/sidekiq/throttled/strategy/base.rb | 2 +- rubocop/layout.yml | 4 ++++ rubocop/rspec.yml | 9 +++++---- rubocop/style.yml | 2 +- spec/lib/sidekiq/throttled/config_spec.rb | 6 +++--- spec/lib/sidekiq/throttled/cooldown_spec.rb | 2 +- spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb | 2 +- spec/lib/sidekiq/throttled/registry_spec.rb | 2 +- 9 files changed, 18 insertions(+), 13 deletions(-) diff --git a/lib/sidekiq/throttled.rb b/lib/sidekiq/throttled.rb index 86458f38..63972c57 100644 --- a/lib/sidekiq/throttled.rb +++ b/lib/sidekiq/throttled.rb @@ -85,7 +85,7 @@ def throttled?(message) end false - rescue + rescue StandardError false end diff --git a/lib/sidekiq/throttled/strategy/base.rb b/lib/sidekiq/throttled/strategy/base.rb index 5ea016e4..b0ee0730 100644 --- a/lib/sidekiq/throttled/strategy/base.rb +++ b/lib/sidekiq/throttled/strategy/base.rb @@ -15,7 +15,7 @@ def key(job_args) return key unless @key_suffix key << ":#{@key_suffix.call(*job_args)}" - rescue => e + rescue StandardError => e Sidekiq.logger.error "Failed to get key suffix: #{e}" raise e end diff --git a/rubocop/layout.yml b/rubocop/layout.yml index 9d97320d..6e5c7f1d 100644 --- a/rubocop/layout.yml +++ b/rubocop/layout.yml @@ -5,6 +5,10 @@ Layout/ArgumentAlignment: Layout/EmptyLinesAroundAttributeAccessor: Enabled: true +Layout/FirstArrayElementIndentation: + Enabled: true + EnforcedStyle: consistent + Layout/FirstHashElementIndentation: Enabled: true EnforcedStyle: consistent diff --git a/rubocop/rspec.yml b/rubocop/rspec.yml index 8ca037ed..e0155c94 100644 --- a/rubocop/rspec.yml +++ b/rubocop/rspec.yml @@ -1,13 +1,14 @@ RSpec/ExampleLength: Enabled: true Max: 10 - -RSpec/ExpectChange: - Enabled: true - EnforcedStyle: block + CountAsOne: [array, hash, heredoc, method_call] RSpec/MultipleExpectations: Enabled: false RSpec/NamedSubject: Enabled: false + +RSpec/BeNil: + Enabled: true + EnforcedStyle: be diff --git a/rubocop/style.yml b/rubocop/style.yml index 18f14c29..3a7e90f7 100644 --- a/rubocop/style.yml +++ b/rubocop/style.yml @@ -67,7 +67,7 @@ Style/RegexpLiteral: Style/RescueStandardError: Enabled: true - EnforcedStyle: implicit + EnforcedStyle: explicit Style/SingleArgumentDig: Enabled: true diff --git a/spec/lib/sidekiq/throttled/config_spec.rb b/spec/lib/sidekiq/throttled/config_spec.rb index 6503f5f7..ef6af48d 100644 --- a/spec/lib/sidekiq/throttled/config_spec.rb +++ b/spec/lib/sidekiq/throttled/config_spec.rb @@ -14,12 +14,12 @@ describe "#cooldown_period=" do it "updates #cooldown_period" do expect { config.cooldown_period = 42.0 } - .to change { config.cooldown_period }.to(42.0) + .to change(config, :cooldown_period).to(42.0) end it "allows setting value to `nil`" do expect { config.cooldown_period = nil } - .to change { config.cooldown_period }.to(nil) + .to change(config, :cooldown_period).to(nil) end it "fails if given value is neither `NilClass` nor `Float`" do @@ -42,7 +42,7 @@ describe "#cooldown_threshold=" do it "updates #cooldown_threshold" do expect { config.cooldown_threshold = 42 } - .to change { config.cooldown_threshold }.to(42) + .to change(config, :cooldown_threshold).to(42) end it "fails if given value is not `Integer`" do diff --git a/spec/lib/sidekiq/throttled/cooldown_spec.rb b/spec/lib/sidekiq/throttled/cooldown_spec.rb index 9472259b..c2c1e9a7 100644 --- a/spec/lib/sidekiq/throttled/cooldown_spec.rb +++ b/spec/lib/sidekiq/throttled/cooldown_spec.rb @@ -15,7 +15,7 @@ context "when `cooldown_period` is nil" do before { config.cooldown_period = nil } - it { is_expected.to be_nil } + it { is_expected.to be(nil) } end end diff --git a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb index 680aa8e2..72574652 100644 --- a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb +++ b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb @@ -65,7 +65,7 @@ def enqueued_jobs(queue) it "returns nothing" do fetch.retrieve_work - expect(fetch.retrieve_work).to be_nil + expect(fetch.retrieve_work).to be(nil) end it "pushes job back to the head of the queue" do diff --git a/spec/lib/sidekiq/throttled/registry_spec.rb b/spec/lib/sidekiq/throttled/registry_spec.rb index f7231585..84041418 100644 --- a/spec/lib/sidekiq/throttled/registry_spec.rb +++ b/spec/lib/sidekiq/throttled/registry_spec.rb @@ -59,7 +59,7 @@ def stub_class(name, *parent, &block) let(:name) { "foo" } context "when strategy is not registered" do - it { is_expected.to be_nil } + it { is_expected.to be(nil) } end context "when strategy was registered" do From e8eccd48b19f8c0bf044dd77572333c25299c0f7 Mon Sep 17 00:00:00 2001 From: Alexandr Elhovenko Date: Mon, 18 Dec 2023 21:26:59 +0000 Subject: [PATCH 31/62] feat: Add back Ruby-2.7 support We will drop Ruby-2.7 support unconditionally after Ruby-3.0 EOL. Signed-off-by: Alexey Zapparov Co-authored-by: Alexey Zapparov --- .github/workflows/ci.yml | 2 +- .rubocop.yml | 2 +- CHANGES.md | 7 +++++++ README.adoc | 1 + lib/sidekiq/throttled/version.rb | 2 +- sidekiq-throttled.gemspec | 2 +- 6 files changed, 12 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 861be1de..0154d47c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,7 +26,7 @@ jobs: strategy: fail-fast: false matrix: - ruby: [ ruby-3.0, ruby-3.1, ruby-3.2 ] + ruby: [ ruby-2.7, ruby-3.0, ruby-3.1, ruby-3.2 ] steps: - uses: actions/checkout@v4 diff --git a/.rubocop.yml b/.rubocop.yml index 90e5e296..a2e7d7c8 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -18,7 +18,7 @@ AllCops: - gemfiles/**/* - vendor/**/* NewCops: enable - TargetRubyVersion: 3.0 + TargetRubyVersion: 2.7 # Broken: https://github.com/rubocop/rubocop/issues/12113 Bundler/DuplicatedGroup: diff --git a/CHANGES.md b/CHANGES.md index 1d665ab5..8911388a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [1.2.0] - 2023-12-18 + +### Added + +- Bring back Ruby-2.7.x support + + ## [1.1.0] - 2023-11-21 ### Changed diff --git a/README.adoc b/README.adoc index 8f28415c..2680a8ae 100644 --- a/README.adoc +++ b/README.adoc @@ -267,6 +267,7 @@ sidekiq_throttle(concurrency: { limit: 20, ttl: 1.hour.to_i }) This library aims to support and is tested against the following Ruby versions: +* Ruby 2.7.x * Ruby 3.0.x * Ruby 3.1.x * Ruby 3.2.x diff --git a/lib/sidekiq/throttled/version.rb b/lib/sidekiq/throttled/version.rb index f6306329..0b58616b 100644 --- a/lib/sidekiq/throttled/version.rb +++ b/lib/sidekiq/throttled/version.rb @@ -3,6 +3,6 @@ module Sidekiq module Throttled # Gem version - VERSION = "1.1.0" + VERSION = "1.2.0" end end diff --git a/sidekiq-throttled.gemspec b/sidekiq-throttled.gemspec index 8b9a1122..34677fd6 100644 --- a/sidekiq-throttled.gemspec +++ b/sidekiq-throttled.gemspec @@ -30,7 +30,7 @@ Gem::Specification.new do |spec| spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) } spec.require_paths = ["lib"] - spec.required_ruby_version = ">= 3.0" + spec.required_ruby_version = ">= 2.7" spec.add_runtime_dependency "concurrent-ruby", ">= 1.2.0" spec.add_runtime_dependency "redis-prescription", "~> 2.2" From cafb54290d5a7c6c0e9a21e186934eef4df988b3 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Mon, 18 Dec 2023 22:32:57 +0100 Subject: [PATCH 32/62] ci: Improve output of test matrix --- .github/workflows/ci.yml | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0154d47c..3d738863 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,13 +8,13 @@ on: jobs: test: - name: "rspec (ruby:${{ matrix.ruby }})" + name: "test / ${{ matrix.ruby }} / ${{ matrix.redis }}" runs-on: ubuntu-latest services: redis: - image: redis + image: ${{ matrix.redis }} ports: - "6379:6379" options: >- @@ -27,6 +27,7 @@ jobs: fail-fast: false matrix: ruby: [ ruby-2.7, ruby-3.0, ruby-3.1, ruby-3.2 ] + redis: [ "redis:6.2", "redis:7.0", "redis:7.2" ] steps: - uses: actions/checkout@v4 @@ -43,6 +44,18 @@ jobs: env: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + # See: https://github.com/orgs/community/discussions/26822#discussioncomment-3305794 + test-finale: + name: "test" + + runs-on: ubuntu-latest + if: ${{ always() }} + + needs: [test] + + steps: + - run: test "success" = "${{ needs.test.result }}" + lint: runs-on: ubuntu-latest From ff43b390bef772d9fd651cd27efc32db5c70d266 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Tue, 16 Jan 2024 05:27:18 +0100 Subject: [PATCH 33/62] feat: Add Ruby-3.3 support Extracted from https://github.com/ixti/sidekiq-throttled/pull/175 --- .github/workflows/ci.yml | 2 +- CHANGES.md | 2 ++ README.adoc | 6 +++--- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3d738863..7c4ee94c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,7 +26,7 @@ jobs: strategy: fail-fast: false matrix: - ruby: [ ruby-2.7, ruby-3.0, ruby-3.1, ruby-3.2 ] + ruby: [ ruby-2.7, ruby-3.0, ruby-3.1, ruby-3.2, ruby-3.3 ] redis: [ "redis:6.2", "redis:7.0", "redis:7.2" ] steps: diff --git a/CHANGES.md b/CHANGES.md index 8911388a..bcd858ba 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- Add Ruby 3.3 support + ## [1.2.0] - 2023-12-18 ### Added diff --git a/README.adoc b/README.adoc index 2680a8ae..f184b4df 100644 --- a/README.adoc +++ b/README.adoc @@ -107,7 +107,7 @@ end * `Sidekiq::Throttled::Middlewares::Server` The middleware is automatically injected when you require `sidekiq/throttled`. -In rare cases this might be an issue. You can change to order manually: +In rare cases, when this causes an issue, you can change middleware order manually: [source,ruby] ---- @@ -115,8 +115,7 @@ Sidekiq.configure_server do |config| # ... config.server_middleware do |chain| - chain.remove(Sidekiq::Throttled::Middlewares::Server) - chain.add(Sidekiq::Throttled::Middlewares::Server) + chain.prepend(Sidekiq::Throttled::Middlewares::Server) end end ---- @@ -271,6 +270,7 @@ This library aims to support and is tested against the following Ruby versions: * Ruby 3.0.x * Ruby 3.1.x * Ruby 3.2.x +* Ruby 3.3.x If something doesn't work on one of these versions, it's a bug. From 02704ab4a41daf77be12f37e215c0cb37511db18 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Tue, 16 Jan 2024 05:48:55 +0100 Subject: [PATCH 34/62] lint: Simplify args proxying --- lib/sidekiq/throttled/strategy_collection.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/sidekiq/throttled/strategy_collection.rb b/lib/sidekiq/throttled/strategy_collection.rb index ce9d7e34..71a8afa7 100644 --- a/lib/sidekiq/throttled/strategy_collection.rb +++ b/lib/sidekiq/throttled/strategy_collection.rb @@ -26,8 +26,8 @@ def initialize(strategies, strategy:, name:, key_suffix:) # @param [#call] block # Iterates each strategy in collection - def each(&block) - @strategies.each(&block) + def each(...) + @strategies.each(...) end # @return [Boolean] whenever any strategy in collection has dynamic config @@ -37,14 +37,14 @@ def dynamic? # @return [Boolean] whenever job is throttled or not # by any strategy in collection. - def throttled?(*args) - any? { |s| s.throttled?(*args) } + def throttled?(...) + any? { |s| s.throttled?(...) } end # Marks job as being processed. # @return [void] - def finalize!(*args) - each { |c| c.finalize!(*args) } + def finalize!(...) + each { |c| c.finalize!(...) } end # Resets count of jobs of all avaliable strategies From 6294b2b91066b31168e52b8f9553ba60f7f99bdd Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Tue, 16 Jan 2024 16:48:45 +0100 Subject: [PATCH 35/62] refactor: Extract retrieve_work patch This should help DRY-up #175 --- lib/sidekiq/throttled/patches/basic_fetch.rb | 19 +++----------- .../throttled/patches/throttled_retriever.rb | 26 +++++++++++++++++++ 2 files changed, 30 insertions(+), 15 deletions(-) create mode 100644 lib/sidekiq/throttled/patches/throttled_retriever.rb diff --git a/lib/sidekiq/throttled/patches/basic_fetch.rb b/lib/sidekiq/throttled/patches/basic_fetch.rb index 0f933dc8..53ef9e90 100644 --- a/lib/sidekiq/throttled/patches/basic_fetch.rb +++ b/lib/sidekiq/throttled/patches/basic_fetch.rb @@ -3,25 +3,14 @@ require "sidekiq" require "sidekiq/fetch" +require_relative "./throttled_retriever" + module Sidekiq module Throttled module Patches module BasicFetch - # Retrieves job from redis. - # - # @return [Sidekiq::Throttled::UnitOfWork, nil] - def retrieve_work - work = super - - if work && Throttled.throttled?(work.job) - Throttled.cooldown&.notify_throttled(work.queue) - requeue_throttled(work) - return nil - end - - Throttled.cooldown&.notify_admitted(work.queue) if work - - work + def self.prepended(base) + base.prepend(ThrottledRetriever) end private diff --git a/lib/sidekiq/throttled/patches/throttled_retriever.rb b/lib/sidekiq/throttled/patches/throttled_retriever.rb new file mode 100644 index 00000000..353c58fa --- /dev/null +++ b/lib/sidekiq/throttled/patches/throttled_retriever.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module Sidekiq + module Throttled + module Patches + module ThrottledRetriever + # Retrieves job from redis. + # + # @return [Sidekiq::BasicFetch::UnitOfWork, nil] + def retrieve_work + work = super + + if work && Throttled.throttled?(work.job) + Throttled.cooldown&.notify_throttled(work.queue) + requeue_throttled(work) + return nil + end + + Throttled.cooldown&.notify_admitted(work.queue) if work + + work + end + end + end + end +end From bf88735d377d1d9b18ea2d450c78bc24af761fe6 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Tue, 16 Jan 2024 17:39:25 +0100 Subject: [PATCH 36/62] refactor: Reduce arrays allocation --- lib/sidekiq/throttled/patches/basic_fetch.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/sidekiq/throttled/patches/basic_fetch.rb b/lib/sidekiq/throttled/patches/basic_fetch.rb index 53ef9e90..5acb3971 100644 --- a/lib/sidekiq/throttled/patches/basic_fetch.rb +++ b/lib/sidekiq/throttled/patches/basic_fetch.rb @@ -32,7 +32,10 @@ def requeue_throttled(work) # @param [Array] queues # @return [Array] def queues_cmd - super - (Throttled.cooldown&.queues || []) + throttled_queues = Throttled.cooldown&.queues + return super unless throttled_queues&.size&.positive? + + super - throttled_queues end end end From ecc602b768df9b58303e0363fbd7002f79464ce0 Mon Sep 17 00:00:00 2001 From: Mauricio Novelo Date: Sat, 13 Jan 2024 18:39:56 -0500 Subject: [PATCH 37/62] feat: Support Sidekiq-Pro super fetch Closes: #178 Resolves: #111 Co-authored-by: Alexey Zapparov Signed-off-by: Mauricio Novelo Signed-off-by: Alexey Zapparov --- .github/codecov.yml | 4 + .github/workflows/ci.yml | 3 + .rspec | 1 + .rspec.sidekiq-pro | 2 + Appraisals | 24 +++++ CHANGES.md | 1 + README.adoc | 10 ++ lib/sidekiq/throttled.rb | 1 + lib/sidekiq/throttled/patches/basic_fetch.rb | 2 +- lib/sidekiq/throttled/patches/super_fetch.rb | 52 ++++++++++ .../throttled/patches/basic_fetch_spec.rb | 23 ----- .../throttled/patches/super_fetch_spec.rb | 98 +++++++++++++++++++ spec/support/sidekiq.rb | 82 ++++++++++++++-- 13 files changed, 271 insertions(+), 32 deletions(-) create mode 100644 .github/codecov.yml create mode 100644 .rspec.sidekiq-pro create mode 100644 lib/sidekiq/throttled/patches/super_fetch.rb create mode 100644 spec/lib/sidekiq/throttled/patches/super_fetch_spec.rb diff --git a/.github/codecov.yml b/.github/codecov.yml new file mode 100644 index 00000000..82b0def4 --- /dev/null +++ b/.github/codecov.yml @@ -0,0 +1,4 @@ +ignore: + # Requires Sidekiq-Pro + - lib/sidekiq/throttled/patches/super_fetch.rb + - spec/lib/sidekiq/throttled/patches/super_fetch_spec.rb diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7c4ee94c..377a8391 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,6 +23,9 @@ jobs: --health-timeout 5s --health-retries 5 + env: + BUNDLE_GEMS__CONTRIBSYS__COM: ${{ secrets.BUNDLE_GEMS__CONTRIBSYS__COM }} + strategy: fail-fast: false matrix: diff --git a/.rspec b/.rspec index 283edea7..5f2fd638 100644 --- a/.rspec +++ b/.rspec @@ -1,2 +1,3 @@ --require simplecov --require spec_helper +--tag ~sidekiq_pro diff --git a/.rspec.sidekiq-pro b/.rspec.sidekiq-pro new file mode 100644 index 00000000..283edea7 --- /dev/null +++ b/.rspec.sidekiq-pro @@ -0,0 +1,2 @@ +--require simplecov +--require spec_helper diff --git a/Appraisals b/Appraisals index c62504a1..c2f061f8 100644 --- a/Appraisals +++ b/Appraisals @@ -9,17 +9,41 @@ end appraise "sidekiq-7.0.x" do group :test do gem "sidekiq", "~> 7.0.0" + + # Sidekiq Pro license must be set in global bundler config + # or in BUNDLE_GEMS__CONTRIBSYS__COM env variable + install_if "-> { Bundler.settings['gems.contribsys.com']&.include?(':') || ENV['BUNDLE_GEMS__CONTRIBSYS__COM'] }" do + source "https://gems.contribsys.com/" do + gem "sidekiq-pro", "~> 7.0.0" + end + end end end appraise "sidekiq-7.1.x" do group :test do gem "sidekiq", "~> 7.1.0" + + # Sidekiq Pro license must be set in global bundler config + # or in BUNDLE_GEMS__CONTRIBSYS__COM env variable + install_if "-> { Bundler.settings['gems.contribsys.com']&.include?(':') || ENV['BUNDLE_GEMS__CONTRIBSYS__COM'] }" do + source "https://gems.contribsys.com/" do + gem "sidekiq-pro", "~> 7.1.0" + end + end end end appraise "sidekiq-7.2.x" do group :test do gem "sidekiq", "~> 7.2.0" + + # Sidekiq Pro license must be set in global bundler config + # or in BUNDLE_GEMS__CONTRIBSYS__COM env variable + install_if "-> { Bundler.settings['gems.contribsys.com']&.include?(':') || ENV['BUNDLE_GEMS__CONTRIBSYS__COM'] }" do + source "https://gems.contribsys.com/" do + gem "sidekiq-pro", "~> 7.2.0" + end + end end end diff --git a/CHANGES.md b/CHANGES.md index bcd858ba..5d9b2dac 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- Add Sidekiq Pro 7.0, 7.1, and 7.2 support - Add Ruby 3.3 support ## [1.2.0] - 2023-12-18 diff --git a/README.adoc b/README.adoc index f184b4df..2845780e 100644 --- a/README.adoc +++ b/README.adoc @@ -295,6 +295,11 @@ This library aims to support and work with following Sidekiq versions: * Sidekiq 7.1.x * Sidekiq 7.2.x +And the following Sidekiq Pro versions: + +* Sidekiq Pro 7.0.x +* Sidekiq Pro 7.1.x +* Sidekiq Pro 7.2.x == Development @@ -303,6 +308,11 @@ This library aims to support and work with following Sidekiq versions: bundle exec appraisal install bundle exec rake +=== Sidekiq-Pro + +If you're working on Sidekiq-Pro support make sure to copy `.rspec-sidekiq-pro` +to `.rspec-local` and that you have Sidekiq-Pro license in the global config, +or in the `BUNDLE_GEMS__CONTRIBSYS__COM` env variable. == Contributing diff --git a/lib/sidekiq/throttled.rb b/lib/sidekiq/throttled.rb index 63972c57..38915cae 100644 --- a/lib/sidekiq/throttled.rb +++ b/lib/sidekiq/throttled.rb @@ -7,6 +7,7 @@ require_relative "./throttled/job" require_relative "./throttled/middlewares/server" require_relative "./throttled/patches/basic_fetch" +require_relative "./throttled/patches/super_fetch" require_relative "./throttled/registry" require_relative "./throttled/version" require_relative "./throttled/worker" diff --git a/lib/sidekiq/throttled/patches/basic_fetch.rb b/lib/sidekiq/throttled/patches/basic_fetch.rb index 5acb3971..83d079be 100644 --- a/lib/sidekiq/throttled/patches/basic_fetch.rb +++ b/lib/sidekiq/throttled/patches/basic_fetch.rb @@ -33,7 +33,7 @@ def requeue_throttled(work) # @return [Array] def queues_cmd throttled_queues = Throttled.cooldown&.queues - return super unless throttled_queues&.size&.positive? + return super if throttled_queues.nil? || throttled_queues.empty? super - throttled_queues end diff --git a/lib/sidekiq/throttled/patches/super_fetch.rb b/lib/sidekiq/throttled/patches/super_fetch.rb new file mode 100644 index 00000000..c1099cd8 --- /dev/null +++ b/lib/sidekiq/throttled/patches/super_fetch.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require "sidekiq" + +require_relative "./throttled_retriever" + +module Sidekiq + module Throttled + module Patches + module SuperFetch + def self.prepended(base) + base.prepend(ThrottledRetriever) + end + + private + + # Calls SuperFetch UnitOfWork's requeue to remove the job from the + # temporary queue and push job back to the head of the queue, so that + # the job won't be tried immediately after it was requeued (in most cases). + # + # @note This is triggered when job is throttled. + # + # @return [void] + def requeue_throttled(work) + # SuperFetch UnitOfWork's requeue will remove it from the temporary + # queue and then requeue it, so no acknowledgement call is needed. + work.requeue + end + + # Returns list of non-paused queues to try to fetch jobs from. + # + # @note It may return an empty array. + # @return [Array] + def active_queues + # Create a hash of throttled queues for fast lookup + throttled_queues = Throttled.cooldown&.queues&.to_h { |queue| [queue, true] } + return super if throttled_queues.nil? || throttled_queues.empty? + + # Reject throttled queues from the list of active queues + super.reject { |queue, _private_queue| throttled_queues[queue] } + end + end + end + end +end + +begin + require "sidekiq/pro/super_fetch" + Sidekiq::Pro::SuperFetch.prepend(Sidekiq::Throttled::Patches::SuperFetch) +rescue LoadError + # Sidekiq Pro is not available +end diff --git a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb index 72574652..d5fa1ebc 100644 --- a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb +++ b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb @@ -3,29 +3,6 @@ require "sidekiq/throttled/patches/basic_fetch" RSpec.describe Sidekiq::Throttled::Patches::BasicFetch do - def stub_job_class(name, &block) - klass = stub_const(name, Class.new) - - klass.include(Sidekiq::Job) - klass.include(Sidekiq::Throttled::Job) - - klass.instance_exec do - def perform(*); end - end - - klass.instance_exec(&block) if block - end - - def enqueued_jobs(queue) - Sidekiq.redis do |conn| - conn.lrange("queue:#{queue}", 0, -1).map do |job| - JSON.parse(job).then do |payload| - [payload["class"], *payload["args"]] - end - end - end - end - let(:fetch) do if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") Sidekiq.instance_variable_set(:@config, Sidekiq::DEFAULTS.dup) diff --git a/spec/lib/sidekiq/throttled/patches/super_fetch_spec.rb b/spec/lib/sidekiq/throttled/patches/super_fetch_spec.rb new file mode 100644 index 00000000..784add02 --- /dev/null +++ b/spec/lib/sidekiq/throttled/patches/super_fetch_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +require "sidekiq/throttled/patches/super_fetch" + +RSpec.describe Sidekiq::Throttled::Patches::SuperFetch, :sidekiq_pro do + let(:base_queue) { "default" } + let(:critical_queue) { "critical" } + let(:config) do + config = Sidekiq.instance_variable_get(:@config) + config.super_fetch! + config.queues = [base_queue, critical_queue] + config + end + let(:fetch) do + config.default_capsule.fetcher + end + + before do + Sidekiq::Throttled.configure { |config| config.cooldown_period = nil } + + bq = base_queue + cq = critical_queue + stub_job_class("TestJob") { sidekiq_options(queue: bq) } + stub_job_class("AnotherTestJob") { sidekiq_options(queue: cq) } + + allow(Process).to receive(:clock_gettime).with(Process::CLOCK_MONOTONIC).and_return(0.0) + + # Give super_fetch a chance to finish its initialization, but also check that there are no pre-existing jobs + pre_existing_job = fetch.retrieve_work + raise "Found pre-existing job: #{pre_existing_job.inspect}" if pre_existing_job + + # Sidekiq is FIFO queue, with head on right side of the list, + # meaning jobs below will be stored in 3, 2, 1 order. + TestJob.perform_bulk([[1], [2], [3]]) + AnotherTestJob.perform_async(4) + end + + describe "#retrieve_work" do + context "when job is not throttled" do + it "returns unit of work" do + expect(Array.new(4) { fetch.retrieve_work }).to all be_an_instance_of(Sidekiq::Pro::SuperFetch::UnitOfWork) + end + end + + shared_examples "requeues throttled job" do + it "returns nothing" do + fetch.retrieve_work + + expect(fetch.retrieve_work).to be(nil) + end + + it "pushes job back to the head of the queue" do + fetch.retrieve_work + + expect { fetch.retrieve_work } + .to change { enqueued_jobs(base_queue) }.to([["TestJob", 2], ["TestJob", 3]]) + .and(keep_unchanged { enqueued_jobs(critical_queue) }) + end + + context "when queue cooldown kicks in" do + before do + Sidekiq::Throttled.configure do |config| + config.cooldown_period = 2.0 + config.cooldown_threshold = 1 + end + + fetch.retrieve_work + end + + it "updates cooldown queues" do + expect { fetch.retrieve_work } + .to change { enqueued_jobs(base_queue) }.to([["TestJob", 2], ["TestJob", 3]]) + .and(change { Sidekiq::Throttled.cooldown.queues }.to(["queue:#{base_queue}"])) + end + + it "excludes the queue from polling" do + fetch.retrieve_work + + expect { fetch.retrieve_work } + .to change { enqueued_jobs(critical_queue) }.to([]) + .and(keep_unchanged { enqueued_jobs(base_queue) }) + end + end + end + + context "when job was throttled due to concurrency" do + before { TestJob.sidekiq_throttle(concurrency: { limit: 1 }) } + + include_examples "requeues throttled job" + end + + context "when job was throttled due to threshold" do + before { TestJob.sidekiq_throttle(threshold: { limit: 1, period: 60 }) } + + include_examples "requeues throttled job" + end + end +end diff --git a/spec/support/sidekiq.rb b/spec/support/sidekiq.rb index 95a1b013..36e0c8c0 100644 --- a/spec/support/sidekiq.rb +++ b/spec/support/sidekiq.rb @@ -8,11 +8,81 @@ require "sidekiq" require "sidekiq/cli" -$TESTING = false # rubocop:disable Style/GlobalVars +begin + require "sidekiq-pro" +rescue LoadError + # Sidekiq Pro is not available +end + +$TESTING = true # rubocop:disable Style/GlobalVars REDIS_URL = ENV.fetch("REDIS_URL", "redis://localhost:6379") -module JidGenerator +module SidekiqThrottledHelper + def new_sidekiq_config + cfg = Sidekiq::Config.new + cfg.redis = { url: REDIS_URL } + cfg.logger = PseudoLogger.instance + cfg.logger.level = Logger::WARN + cfg.server_middleware do |chain| + chain.add(Sidekiq::Throttled::Middlewares::Server) + end + cfg + end + + def reset_redis! + if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") + reset_redis_v6! + else + reset_redis_v7! + end + end + + def reset_redis_v6! + Sidekiq.redis do |conn| + conn.flushdb + conn.script("flush") + end + end + + # Resets Sidekiq config between tests like mperham does in Sidekiq tests: + # https://github.com/sidekiq/sidekiq/blob/7df28434f03fa1111e9e2834271c020205369f94/test/helper.rb#L30 + def reset_redis_v7! + if Sidekiq.default_configuration.instance_variable_defined?(:@redis) + existing_pool = Sidekiq.default_configuration.instance_variable_get(:@redis) + existing_pool&.shutdown(&:close) + end + + RedisClient.new(url: REDIS_URL).call("flushall") + + # After resetting redis, create a new Sidekiq::Config instance to avoid ConnectionPool::PoolShuttingDownError + Sidekiq.instance_variable_set :@config, new_sidekiq_config + new_sidekiq_config + end + + def stub_job_class(name, &block) + klass = stub_const(name, Class.new) + + klass.include(Sidekiq::Job) + klass.include(Sidekiq::Throttled::Job) + + klass.instance_exec do + def perform(*); end + end + + klass.instance_exec(&block) if block + end + + def enqueued_jobs(queue) + Sidekiq.redis do |conn| + conn.lrange("queue:#{queue}", 0, -1).map do |job| + JSON.parse(job).then do |payload| + [payload["class"], *payload["args"]] + end + end + end + end + def jid SecureRandom.hex 12 end @@ -54,15 +124,11 @@ def output end RSpec.configure do |config| - config.include JidGenerator - config.extend JidGenerator + config.include SidekiqThrottledHelper config.before do PseudoLogger.instance.reset! - Sidekiq.redis do |conn| - conn.flushdb - conn.script("flush") - end + reset_redis! end end From f0497a58066fb89b36cb586f6e4a3ec9e6aa37a1 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Thu, 18 Jan 2024 17:56:24 +0100 Subject: [PATCH 38/62] doc: Fix markup of environment variable name Signed-off-by: Alexey Zapparov --- README.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.adoc b/README.adoc index 2845780e..6c9c8cb2 100644 --- a/README.adoc +++ b/README.adoc @@ -312,7 +312,7 @@ And the following Sidekiq Pro versions: If you're working on Sidekiq-Pro support make sure to copy `.rspec-sidekiq-pro` to `.rspec-local` and that you have Sidekiq-Pro license in the global config, -or in the `BUNDLE_GEMS__CONTRIBSYS__COM` env variable. +or in the `BUNDLE_GEMS\__CONTRIBSYS__COM` env variable. == Contributing From 659fdfa9b0ef420108dbdf32fd40654c583c8b33 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Thu, 18 Jan 2024 18:00:52 +0100 Subject: [PATCH 39/62] Release v1.3.0 --- CHANGES.md | 10 +++++++++- lib/sidekiq/throttled/version.rb | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 5d9b2dac..f02c82d4 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,9 +8,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] + +## [1.3.0] - 2024-01-18 + +### Added + - Add Sidekiq Pro 7.0, 7.1, and 7.2 support - Add Ruby 3.3 support + ## [1.2.0] - 2023-12-18 ### Added @@ -99,7 +105,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Remove queue exclusion from fetcher pon throttled job -[unreleased]: https://github.com/ixti/sidekiq-throttled/compare/v1.1.0...main +[unreleased]: https://github.com/ixti/sidekiq-throttled/compare/v1.3.0...main +[1.3.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.2.0...v1.3.0 +[1.2.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.1.0...v1.2.0 [1,1.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.1...v1.1.0 [1.0.1]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0...v1.0.1 [1.0.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0.alpha.1...v1.0.0 diff --git a/lib/sidekiq/throttled/version.rb b/lib/sidekiq/throttled/version.rb index 0b58616b..6a0514c2 100644 --- a/lib/sidekiq/throttled/version.rb +++ b/lib/sidekiq/throttled/version.rb @@ -3,6 +3,6 @@ module Sidekiq module Throttled # Gem version - VERSION = "1.2.0" + VERSION = "1.3.0" end end From 8dbc66d2e06db4d9c9eb119b9de615b24bbdf79c Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Thu, 18 Jan 2024 18:13:21 +0100 Subject: [PATCH 40/62] doc: Fix changelog link of v1.1.0 Signed-off-by: Alexey Zapparov --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index f02c82d4..ff9ba29c 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -108,7 +108,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [unreleased]: https://github.com/ixti/sidekiq-throttled/compare/v1.3.0...main [1.3.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.2.0...v1.3.0 [1.2.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.1.0...v1.2.0 -[1,1.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.1...v1.1.0 +[1.1.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.1...v1.1.0 [1.0.1]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0...v1.0.1 [1.0.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0.alpha.1...v1.0.0 [1.0.0.alpha.1]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.0.alpha...v1.0.0.alpha.1 From b4027cc4836059d6a514a8b0d2022bb8f0a0f6af Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Fri, 19 Jan 2024 05:40:02 +0100 Subject: [PATCH 41/62] ci: Run Sidekiq-Pro test suites if license present (#179) --- .github/workflows/ci.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 377a8391..acc1a31b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,7 +40,13 @@ jobs: ruby-version: ${{ matrix.ruby }} bundler-cache: true - - run: bundle exec rake test + - name: Run test suites + run: | + if [[ -n "${BUNDLE_GEMS__CONTRIBSYS__COM}" ]]; then + cp .rspec.sidekiq-pro .rspec-local + fi + + bundle exec rake test - name: Upload coverage reports to Codecov uses: codecov/codecov-action@v3 From 6b60bdd8264dc41ab5d38f1c6d42a21ebc5ca215 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Sat, 27 Jan 2024 00:06:58 +0100 Subject: [PATCH 42/62] ci: Refactor Sidekiq-Pro tests conditionals (#182) Kudos to @mnovelo and @gstokkink for catching this and coming with proposals on fixes. Closes: https://github.com/ixti/sidekiq-throttled/pull/181 Related-To: https://github.com/ixti/sidekiq-throttled/pull/180 --------- Signed-off-by: Alexey Zapparov Co-authored-by: Mauricio Novelo <5916364+mnovelo@users.noreply.github.com> --- .github/workflows/ci.yml | 7 +------ .rspec | 1 - .rspec.sidekiq-pro | 2 -- Appraisals | 6 +++--- README.adoc | 6 +++--- Rakefile | 2 +- spec/spec_helper.rb | 7 +++++++ spec/support/sidekiq.rb | 13 +++++++------ 8 files changed, 22 insertions(+), 22 deletions(-) delete mode 100644 .rspec.sidekiq-pro diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index acc1a31b..e353190d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,12 +41,7 @@ jobs: bundler-cache: true - name: Run test suites - run: | - if [[ -n "${BUNDLE_GEMS__CONTRIBSYS__COM}" ]]; then - cp .rspec.sidekiq-pro .rspec-local - fi - - bundle exec rake test + run: bundle exec rake test - name: Upload coverage reports to Codecov uses: codecov/codecov-action@v3 diff --git a/.rspec b/.rspec index 5f2fd638..283edea7 100644 --- a/.rspec +++ b/.rspec @@ -1,3 +1,2 @@ --require simplecov --require spec_helper ---tag ~sidekiq_pro diff --git a/.rspec.sidekiq-pro b/.rspec.sidekiq-pro deleted file mode 100644 index 283edea7..00000000 --- a/.rspec.sidekiq-pro +++ /dev/null @@ -1,2 +0,0 @@ ---require simplecov ---require spec_helper diff --git a/Appraisals b/Appraisals index c2f061f8..f16caa5c 100644 --- a/Appraisals +++ b/Appraisals @@ -12,7 +12,7 @@ appraise "sidekiq-7.0.x" do # Sidekiq Pro license must be set in global bundler config # or in BUNDLE_GEMS__CONTRIBSYS__COM env variable - install_if "-> { Bundler.settings['gems.contribsys.com']&.include?(':') || ENV['BUNDLE_GEMS__CONTRIBSYS__COM'] }" do + install_if "-> { Bundler.settings['gems.contribsys.com']&.include?(':') }" do source "https://gems.contribsys.com/" do gem "sidekiq-pro", "~> 7.0.0" end @@ -26,7 +26,7 @@ appraise "sidekiq-7.1.x" do # Sidekiq Pro license must be set in global bundler config # or in BUNDLE_GEMS__CONTRIBSYS__COM env variable - install_if "-> { Bundler.settings['gems.contribsys.com']&.include?(':') || ENV['BUNDLE_GEMS__CONTRIBSYS__COM'] }" do + install_if "-> { Bundler.settings['gems.contribsys.com']&.include?(':') }" do source "https://gems.contribsys.com/" do gem "sidekiq-pro", "~> 7.1.0" end @@ -40,7 +40,7 @@ appraise "sidekiq-7.2.x" do # Sidekiq Pro license must be set in global bundler config # or in BUNDLE_GEMS__CONTRIBSYS__COM env variable - install_if "-> { Bundler.settings['gems.contribsys.com']&.include?(':') || ENV['BUNDLE_GEMS__CONTRIBSYS__COM'] }" do + install_if "-> { Bundler.settings['gems.contribsys.com']&.include?(':') }" do source "https://gems.contribsys.com/" do gem "sidekiq-pro", "~> 7.2.0" end diff --git a/README.adoc b/README.adoc index 6c9c8cb2..c2d3202d 100644 --- a/README.adoc +++ b/README.adoc @@ -310,9 +310,9 @@ And the following Sidekiq Pro versions: === Sidekiq-Pro -If you're working on Sidekiq-Pro support make sure to copy `.rspec-sidekiq-pro` -to `.rspec-local` and that you have Sidekiq-Pro license in the global config, -or in the `BUNDLE_GEMS\__CONTRIBSYS__COM` env variable. +If you're working on Sidekiq-Pro support make sure that you have Sidekiq-Pro +license set either in the global config, or in `BUNDLE_GEMS\__CONTRIBSYS__COM` +environment variable. == Contributing diff --git a/Rakefile b/Rakefile index f8aa96de..2dfa7996 100644 --- a/Rakefile +++ b/Rakefile @@ -7,7 +7,7 @@ task :test do rm_rf "coverage" rm_rf "gemfiles" - Bundler.with_unbundled_env do + Bundler.with_original_env do sh "bundle exec appraisal generate" # XXX: `bundle exec appraisal install` fails on ruby-3.2 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index cb34b713..167bbe57 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -24,6 +24,13 @@ end end + # Sidekiq-Pro related specs require license set in Bundler + unless Bundler.settings["gems.contribsys.com"]&.include?(":") && SIDEKIQ7 + config.define_derived_metadata(sidekiq_pro: true) do |metadata| + metadata[:skip] = "Sidekiq::Pro license not found or not supported" + end + end + # rspec-expectations config goes here. You can use an alternate # assertion/expectation library such as wrong or the stdlib/minitest # assertions if you prefer. diff --git a/spec/support/sidekiq.rb b/spec/support/sidekiq.rb index 36e0c8c0..f29f217d 100644 --- a/spec/support/sidekiq.rb +++ b/spec/support/sidekiq.rb @@ -17,6 +17,7 @@ $TESTING = true # rubocop:disable Style/GlobalVars REDIS_URL = ENV.fetch("REDIS_URL", "redis://localhost:6379") +SIDEKIQ7 = Gem::Version.new("7.0.0") <= Gem::Version.new(Sidekiq::VERSION) module SidekiqThrottledHelper def new_sidekiq_config @@ -31,10 +32,10 @@ def new_sidekiq_config end def reset_redis! - if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") - reset_redis_v6! - else + if SIDEKIQ7 reset_redis_v7! + else + reset_redis_v6! end end @@ -105,12 +106,12 @@ def output end end -if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") - Sidekiq[:queues] = %i[default] -else +if SIDEKIQ7 Sidekiq.configure_server do |config| config.queues = %i[default] end +else + Sidekiq[:queues] = %i[default] end Sidekiq.configure_server do |config| From 1b71da2c8f6a9da79b33bc05eb8adf3f470f39e4 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Sat, 3 Feb 2024 22:56:12 +0100 Subject: [PATCH 43/62] ci: bump codecov/codecov-action (#183) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 3 to 4.
Release notes

Sourced from codecov/codecov-action's releases.

v4.0.0

v4 of the Codecov Action uses the CLI as the underlying upload. The CLI has helped to power new features including local upload, the global upload token, and new upcoming features.

Breaking Changes

  • The Codecov Action runs as a node20 action due to node16 deprecation. See this post from GitHub on how to migrate.
  • Tokenless uploading is unsupported. However, PRs made from forks to the upstream public repos will support tokenless (e.g. contributors to OS projects do not need the upstream repo's Codecov token). This doc shows instructions on how to add the Codecov token.
  • OS platforms have been added, though some may not be automatically detected. To see a list of platforms, see our CLI download page
  • Various arguments to the Action have been changed. Please be aware that the arguments match with the CLI's needs

v3 versions and below will not have access to CLI features (e.g. global upload token, ATS).

What's Changed

... (truncated)

Changelog

Sourced from codecov/codecov-action's changelog.

4.0.0-beta.2

Fixes

  • #1085 not adding -n if empty to do-upload command

4.0.0-beta.1

v4 represents a move from the universal uploader to the Codecov CLI. Although this will unlock new features for our users, the CLI is not yet at feature parity with the universal uploader.

Breaking Changes

  • No current support for aarch64 and alpine architectures.
  • Tokenless uploading is unsuported
  • Various arguments to the Action have been removed

3.1.4

Fixes

  • #967 Fix typo in README.md
  • #971 fix: add back in working dir
  • #969 fix: CLI option names for uploader

Dependencies

  • #970 build(deps-dev): bump @​types/node from 18.15.12 to 18.16.3
  • #979 build(deps-dev): bump @​types/node from 20.1.0 to 20.1.2
  • #981 build(deps-dev): bump @​types/node from 20.1.2 to 20.1.4

3.1.3

Fixes

  • #960 fix: allow for aarch64 build

Dependencies

  • #957 build(deps-dev): bump jest-junit from 15.0.0 to 16.0.0
  • #958 build(deps): bump openpgp from 5.7.0 to 5.8.0
  • #959 build(deps-dev): bump @​types/node from 18.15.10 to 18.15.12

3.1.2

Fixes

  • #718 Update README.md
  • #851 Remove unsupported path_to_write_report argument
  • #898 codeql-analysis.yml
  • #901 Update README to contain correct information - inputs and negate feature
  • #955 fix: add in all the extra arguments for uploader

Dependencies

  • #819 build(deps): bump openpgp from 5.4.0 to 5.5.0
  • #835 build(deps): bump node-fetch from 3.2.4 to 3.2.10
  • #840 build(deps): bump ossf/scorecard-action from 1.1.1 to 2.0.4
  • #841 build(deps): bump @​actions/core from 1.9.1 to 1.10.0
  • #843 build(deps): bump @​actions/github from 5.0.3 to 5.1.1
  • #869 build(deps): bump node-fetch from 3.2.10 to 3.3.0
  • #872 build(deps-dev): bump jest-junit from 13.2.0 to 15.0.0
  • #879 build(deps): bump decode-uri-component from 0.2.0 to 0.2.2

... (truncated)

Commits
  • f30e495 fix: update action.yml (#1240)
  • a7b945c fix: allow for other archs (#1239)
  • 98ab2c5 Update package.json (#1238)
  • 43235cc Update README.md (#1237)
  • 0cf8684 chore(ci): bump to node20 (#1236)
  • 8e1e730 build(deps-dev): bump @​typescript-eslint/eslint-plugin from 6.19.1 to 6.20.0 ...
  • 61293af build(deps-dev): bump @​typescript-eslint/parser from 6.19.1 to 6.20.0 (#1235)
  • 7a070cb build(deps): bump github/codeql-action from 3.23.1 to 3.23.2 (#1231)
  • 9097165 build(deps): bump actions/upload-artifact from 4.2.0 to 4.3.0 (#1232)
  • ac042ea build(deps-dev): bump @​typescript-eslint/eslint-plugin from 6.19.0 to 6.19.1 ...
  • Additional commits viewable in compare view

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=codecov/codecov-action&package-manager=github_actions&previous-version=3&new-version=4)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e353190d..92e7db03 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,7 +44,7 @@ jobs: run: bundle exec rake test - name: Upload coverage reports to Codecov - uses: codecov/codecov-action@v3 + uses: codecov/codecov-action@v4 env: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} From 3c1c54370093d266bdfaf66f91d1dd8ba296a67a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20Lled=C3=B3?= Date: Sat, 16 Mar 2024 04:13:46 +0100 Subject: [PATCH 44/62] Unwrap ActiveJob arguments (#184) When using the `concurrency` strategy with dynamic suffix, I'm getting this error from `sidekiq-throttle`: `ArgumentError: wrong number of arguments (given 1, expected 2+)` This happens because the `key_suffix` I'm using is not receiving the proper arguments. Consider this dummy job: ```ruby class TestJob < ActiveJob::Base include Sidekiq::Throttled::Job queue_as :default sidekiq_throttle( concurrency: { limit: 1, key_suffix: ->(job_class, index, *) { "job_class:#{job_class}/index:#{index}" }, ttl: 1.hour.to_i } ) def perform(job_class, index) puts "Job Performed. job_class: #{job_class}, index: #{index}" end end ``` This follows the documentation, which states that `key_suffix` must accept the same parameters as the job. However, `sidekiq-throttle` fails to unwrap the `ActiveJob` arguments and instead it just calls the `:throttled?` and `:finalize!` methods passing them the arguments in `msg["args"]`, which in `ActiveJob` is an array of JSON objects, like this: ```json { "args": [ { "job_class": "TestJob", "job_id": "96f6c127-1e0c-4d8c-bdaa-c934b5aaded7", "provider_job_id": null, "queue_name": "default", "priority": null, "arguments": ["TestJob", 9], "executions": 0, "exception_executions": {}, "locale": "en", "timezone": null, "enqueued_at": "2024-03-15T11:31:11.464949297Z", "scheduled_at": null } ] } ``` Some other methods in the `Concurrency` class like `:count` or `:reset!` also call the `:key` method, so they are vulnerable to this error too. However, I haven't found any place in the code where these methods are called with any parameter. I wrote a very simple workaround that seems to work fine for my use case, and the test suite passes. --- lib/sidekiq/throttled.rb | 3 +- lib/sidekiq/throttled/middlewares/server.rb | 7 ++-- .../throttled/middlewares/server_spec.rb | 21 ++++++++++++ spec/lib/sidekiq/throttled_spec.rb | 33 +++++++++++++++++++ 4 files changed, 60 insertions(+), 4 deletions(-) diff --git a/lib/sidekiq/throttled.rb b/lib/sidekiq/throttled.rb index 38915cae..222c15ff 100644 --- a/lib/sidekiq/throttled.rb +++ b/lib/sidekiq/throttled.rb @@ -77,12 +77,13 @@ def configure def throttled?(message) message = Sidekiq.load_json(message) job = message.fetch("wrapped") { message["class"] } + args = message.key?("wrapped") ? message.dig("args", 0, "arguments") : message["args"] jid = message["jid"] return false unless job && jid Registry.get(job) do |strategy| - return strategy.throttled?(jid, *message["args"]) + return strategy.throttled?(jid, *args) end false diff --git a/lib/sidekiq/throttled/middlewares/server.rb b/lib/sidekiq/throttled/middlewares/server.rb index 7ded3172..d45f10b1 100644 --- a/lib/sidekiq/throttled/middlewares/server.rb +++ b/lib/sidekiq/throttled/middlewares/server.rb @@ -13,12 +13,13 @@ class Server def call(_worker, msg, _queue) yield ensure - job = msg.fetch("wrapped") { msg["class"] } - jid = msg["jid"] + job = msg.fetch("wrapped") { msg["class"] } + args = msg.key?("wrapped") ? msg.dig("args", 0, "arguments") : msg["args"] + jid = msg["jid"] if job && jid Registry.get job do |strategy| - strategy.finalize!(jid, *msg["args"]) + strategy.finalize!(jid, *args) end end end diff --git a/spec/lib/sidekiq/throttled/middlewares/server_spec.rb b/spec/lib/sidekiq/throttled/middlewares/server_spec.rb index cc6170fc..460fa991 100644 --- a/spec/lib/sidekiq/throttled/middlewares/server_spec.rb +++ b/spec/lib/sidekiq/throttled/middlewares/server_spec.rb @@ -6,7 +6,9 @@ subject(:middleware) { described_class.new } describe "#call" do + let(:args) { ["bar", 1] } let(:payload) { { "class" => "foo", "jid" => "bar" } } + let(:payload_args) { { "class" => "foo", "jid" => "bar", "args" => args } } context "when job class has strategy with concurrency constraint" do let! :strategy do @@ -19,6 +21,11 @@ middleware.call(double, payload, double) { |*| :foobar } end + it "calls #finalize! of queue with jid and args of job being processed" do + expect(strategy).to receive(:finalize!).with "bar", *args + middleware.call(double, payload_args, double) { |*| :foobar } + end + it "returns yields control to the given block" do expect { |b| middleware.call(double, payload, double, &b) } .to yield_control @@ -38,6 +45,14 @@ "jid" => "bar" } end + let(:payload_args) do + { + "class" => "ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper", + "wrapped" => "wrapped-foo", + "args" => [{ "job_class" => "foo", "arguments" => args }], + "jid" => "bar" + } + end let! :strategy do Sidekiq::Throttled::Registry.add payload["wrapped"], @@ -49,6 +64,12 @@ middleware.call(double, payload, double) { |*| :foobar } end + it "calls #finalize! of queue with jid and args of job being processed" do + expect(strategy).to receive(:finalize!).with "bar", *args + middleware.call(double, payload_args, double) { |*| :foobar } + end + + it "returns yields control to the given block" do expect { |b| middleware.call(double, payload, double, &b) } .to yield_control diff --git a/spec/lib/sidekiq/throttled_spec.rb b/spec/lib/sidekiq/throttled_spec.rb index 7d0341d9..b06ee326 100644 --- a/spec/lib/sidekiq/throttled_spec.rb +++ b/spec/lib/sidekiq/throttled_spec.rb @@ -52,6 +52,20 @@ described_class.throttled? message end + it "passes JID and arguments to registered strategy" do + strategy = Sidekiq::Throttled::Registry.add("foo", + threshold: { limit: 1, period: 1 }, + concurrency: { limit: 1 }) + + payload_jid = jid + args = ["foo", 1] + message = %({"class":"foo","jid":#{payload_jid.inspect},"args":#{args.inspect}}) + + expect(strategy).to receive(:throttled?).with payload_jid, *args + + described_class.throttled? message + end + it "unwraps ActiveJob-jobs default sidekiq adapter" do strategy = Sidekiq::Throttled::Registry.add("wrapped-foo", threshold: { limit: 1, period: 1 }, @@ -85,5 +99,24 @@ described_class.throttled? message end + + it "unwraps ActiveJob-jobs job parameters" do + strategy = Sidekiq::Throttled::Registry.add("wrapped-foo", + threshold: { limit: 1, period: 1 }, + concurrency: { limit: 1 }) + + payload_jid = jid + args = ["foo", 1] + message = JSON.dump({ + "class" => "ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper", + "wrapped" => "wrapped-foo", + "args" => [{ "job_class" => "TestJob", "arguments" => args }], + "jid" => payload_jid + }) + + expect(strategy).to receive(:throttled?).with payload_jid, *args + + described_class.throttled? message + end end end From c151789770958ab29d6431be832f0bbf60877463 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Sat, 16 Mar 2024 04:31:10 +0100 Subject: [PATCH 45/62] fix: DRY-up job class/args extraction Related-PR: https://github.com/ixti/sidekiq-throttled/pull/184 --- CHANGES.md | 6 + lib/sidekiq/throttled.rb | 13 +- lib/sidekiq/throttled/message.rb | 32 +++ lib/sidekiq/throttled/middlewares/server.rb | 11 +- rubocop/rspec.yml | 9 +- spec/lib/sidekiq/throttled/message_spec.rb | 183 ++++++++++++++++++ .../throttled/middlewares/server_spec.rb | 27 ++- spec/lib/sidekiq/throttled_spec.rb | 8 +- 8 files changed, 267 insertions(+), 22 deletions(-) create mode 100644 lib/sidekiq/throttled/message.rb create mode 100644 spec/lib/sidekiq/throttled/message_spec.rb diff --git a/CHANGES.md b/CHANGES.md index ff9ba29c..dbe9ff35 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Correctly unwrap `ActiveJob` arguments: + [#184](https://github.com/ixti/sidekiq-throttled/pull/184), + [#185](https://github.com/ixti/sidekiq-throttled/pull/185). + ## [1.3.0] - 2024-01-18 diff --git a/lib/sidekiq/throttled.rb b/lib/sidekiq/throttled.rb index 222c15ff..51cc6fd0 100644 --- a/lib/sidekiq/throttled.rb +++ b/lib/sidekiq/throttled.rb @@ -5,6 +5,7 @@ require_relative "./throttled/config" require_relative "./throttled/cooldown" require_relative "./throttled/job" +require_relative "./throttled/message" require_relative "./throttled/middlewares/server" require_relative "./throttled/patches/basic_fetch" require_relative "./throttled/patches/super_fetch" @@ -75,15 +76,11 @@ def configure # @param [String] message Job's JSON payload # @return [Boolean] def throttled?(message) - message = Sidekiq.load_json(message) - job = message.fetch("wrapped") { message["class"] } - args = message.key?("wrapped") ? message.dig("args", 0, "arguments") : message["args"] - jid = message["jid"] + message = Message.new(message) + return false unless message.job_class && message.job_id - return false unless job && jid - - Registry.get(job) do |strategy| - return strategy.throttled?(jid, *args) + Registry.get(message.job_class) do |strategy| + return strategy.throttled?(message.job_id, *message.job_args) end false diff --git a/lib/sidekiq/throttled/message.rb b/lib/sidekiq/throttled/message.rb new file mode 100644 index 00000000..c5e77344 --- /dev/null +++ b/lib/sidekiq/throttled/message.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Sidekiq + module Throttled + class Message + def initialize(item) + @item = item.is_a?(Hash) ? item : parse(item) + end + + def job_class + @item.fetch("wrapped") { @item["class"] } + end + + def job_args + @item.key?("wrapped") ? @item.dig("args", 0, "arguments") : @item["args"] + end + + def job_id + @item["jid"] + end + + private + + def parse(item) + item = Sidekiq.load_json(item) + item.is_a?(Hash) ? item : {} + rescue JSON::ParserError + {} + end + end + end +end diff --git a/lib/sidekiq/throttled/middlewares/server.rb b/lib/sidekiq/throttled/middlewares/server.rb index d45f10b1..e9bba5a0 100644 --- a/lib/sidekiq/throttled/middlewares/server.rb +++ b/lib/sidekiq/throttled/middlewares/server.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true # internal +require_relative "../message" require_relative "../registry" module Sidekiq @@ -13,13 +14,11 @@ class Server def call(_worker, msg, _queue) yield ensure - job = msg.fetch("wrapped") { msg["class"] } - args = msg.key?("wrapped") ? msg.dig("args", 0, "arguments") : msg["args"] - jid = msg["jid"] + message = Message.new(msg) - if job && jid - Registry.get job do |strategy| - strategy.finalize!(jid, *args) + if message.job_class && message.job_id + Registry.get(message.job_class) do |strategy| + strategy.finalize!(message.job_id, *message.job_args) end end end diff --git a/rubocop/rspec.yml b/rubocop/rspec.yml index e0155c94..7821bda8 100644 --- a/rubocop/rspec.yml +++ b/rubocop/rspec.yml @@ -1,3 +1,7 @@ +RSpec/BeNil: + Enabled: true + EnforcedStyle: be + RSpec/ExampleLength: Enabled: true Max: 10 @@ -9,6 +13,5 @@ RSpec/MultipleExpectations: RSpec/NamedSubject: Enabled: false -RSpec/BeNil: - Enabled: true - EnforcedStyle: be +RSpec/Rails: + Enabled: false diff --git a/spec/lib/sidekiq/throttled/message_spec.rb b/spec/lib/sidekiq/throttled/message_spec.rb new file mode 100644 index 00000000..2f7c849f --- /dev/null +++ b/spec/lib/sidekiq/throttled/message_spec.rb @@ -0,0 +1,183 @@ +# frozen_string_literal: true + +RSpec.describe Sidekiq::Throttled::Message do + subject(:message) do + described_class.new(item) + end + + let(:item) do + { + "class" => "ExcitingJob", + "args" => [42], + "jid" => "deadbeef" + } + end + + describe "#job_class" do + subject { message.job_class } + + it { is_expected.to eq("ExcitingJob") } + + context "with serialized payload" do + let(:item) do + JSON.dump({ + "class" => "ExcitingJob", + "args" => [42], + "jid" => "deadbeef" + }) + end + + it { is_expected.to eq("ExcitingJob") } + end + + context "with ActiveJob payload" do + let(:item) do + { + "class" => "ActiveJob", + "wrapped" => "ExcitingJob", + "args" => [{ "arguments" => [42] }], + "jid" => "deadbeef" + } + end + + it { is_expected.to eq("ExcitingJob") } + end + + context "with serialized ActiveJob payload" do + let(:item) do + JSON.dump({ + "class" => "ActiveJob", + "wrapped" => "ExcitingJob", + "args" => [{ "arguments" => [42] }], + "jid" => "deadbeef" + }) + end + + it { is_expected.to eq("ExcitingJob") } + end + + context "with invalid payload" do + let(:item) { "invalid" } + + it { is_expected.to be nil } + end + + context "with invalid serialized payload" do + let(:item) { JSON.dump("invalid") } + + it { is_expected.to be nil } + end + end + + describe "#job_args" do + subject { message.job_args } + + it { is_expected.to eq([42]) } + + context "with serialized payload" do + let(:item) do + JSON.dump({ + "class" => "ExcitingJob", + "args" => [42], + "jid" => "deadbeef" + }) + end + + it { is_expected.to eq([42]) } + end + + context "with ActiveJob payload" do + let(:item) do + { + "class" => "ActiveJob", + "wrapped" => "ExcitingJob", + "args" => [{ "arguments" => [42] }], + "jid" => "deadbeef" + } + end + + it { is_expected.to eq([42]) } + end + + context "with serialized ActiveJob payload" do + let(:item) do + JSON.dump({ + "class" => "ActiveJob", + "wrapped" => "ExcitingJob", + "args" => [{ "arguments" => [42] }], + "jid" => "deadbeef" + }) + end + + it { is_expected.to eq([42]) } + end + + context "with invalid payload" do + let(:item) { "invalid" } + + it { is_expected.to be nil } + end + + context "with invalid serialized payload" do + let(:item) { JSON.dump("invalid") } + + it { is_expected.to be nil } + end + end + + describe "#job_id" do + subject { message.job_id } + + it { is_expected.to eq("deadbeef") } + + context "with serialized payload" do + let(:item) do + JSON.dump({ + "class" => "ExcitingJob", + "args" => [42], + "jid" => "deadbeef" + }) + end + + it { is_expected.to eq("deadbeef") } + end + + context "with ActiveJob payload" do + let(:item) do + { + "class" => "ActiveJob", + "wrapped" => "ExcitingJob", + "args" => [{ "arguments" => [42] }], + "jid" => "deadbeef" + } + end + + it { is_expected.to eq("deadbeef") } + end + + context "with serialized ActiveJob payload" do + let(:item) do + JSON.dump({ + "class" => "ActiveJob", + "wrapped" => "ExcitingJob", + "args" => [{ "arguments" => [42] }], + "jid" => "deadbeef" + }) + end + + it { is_expected.to eq("deadbeef") } + end + + context "with invalid payload" do + let(:item) { "invalid" } + + it { is_expected.to be nil } + end + + context "with invalid serialized payload" do + let(:item) { JSON.dump("invalid") } + + it { is_expected.to be nil } + end + end +end diff --git a/spec/lib/sidekiq/throttled/middlewares/server_spec.rb b/spec/lib/sidekiq/throttled/middlewares/server_spec.rb index 460fa991..23ff91b4 100644 --- a/spec/lib/sidekiq/throttled/middlewares/server_spec.rb +++ b/spec/lib/sidekiq/throttled/middlewares/server_spec.rb @@ -69,7 +69,6 @@ middleware.call(double, payload_args, double) { |*| :foobar } end - it "returns yields control to the given block" do expect { |b| middleware.call(double, payload, double, &b) } .to yield_control @@ -114,5 +113,31 @@ .to be :foobar end end + + context "when message contains no job class" do + before do + allow(Sidekiq::Throttled::Registry).to receive(:get).and_call_original + payload.delete("class") + end + + it "does not attempt to retrieve any strategy" do + expect { |b| middleware.call(double, payload, double, &b) }.to yield_control + + expect(Sidekiq::Throttled::Registry).not_to receive(:get) + end + end + + context "when message contains no jid" do + before do + allow(Sidekiq::Throttled::Registry).to receive(:get).and_call_original + payload.delete("jid") + end + + it "does not attempt to retrieve any strategy" do + expect { |b| middleware.call(double, payload, double, &b) }.to yield_control + + expect(Sidekiq::Throttled::Registry).not_to receive(:get) + end + end end end diff --git a/spec/lib/sidekiq/throttled_spec.rb b/spec/lib/sidekiq/throttled_spec.rb index b06ee326..a3413514 100644 --- a/spec/lib/sidekiq/throttled_spec.rb +++ b/spec/lib/sidekiq/throttled_spec.rb @@ -54,8 +54,8 @@ it "passes JID and arguments to registered strategy" do strategy = Sidekiq::Throttled::Registry.add("foo", - threshold: { limit: 1, period: 1 }, - concurrency: { limit: 1 }) + threshold: { limit: 1, period: 1 }, + concurrency: { limit: 1 }) payload_jid = jid args = ["foo", 1] @@ -102,8 +102,8 @@ it "unwraps ActiveJob-jobs job parameters" do strategy = Sidekiq::Throttled::Registry.add("wrapped-foo", - threshold: { limit: 1, period: 1 }, - concurrency: { limit: 1 }) + threshold: { limit: 1, period: 1 }, + concurrency: { limit: 1 }) payload_jid = jid args = ["foo", 1] From f6a7b0d7be88ed794f6396d911d22f2d3146caf1 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Sun, 7 Apr 2024 21:34:56 +0200 Subject: [PATCH 46/62] chore: Update rubocop config --- rubocop/rspec.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rubocop/rspec.yml b/rubocop/rspec.yml index 7821bda8..0d754ef5 100644 --- a/rubocop/rspec.yml +++ b/rubocop/rspec.yml @@ -13,5 +13,5 @@ RSpec/MultipleExpectations: RSpec/NamedSubject: Enabled: false -RSpec/Rails: +RSpecRails: Enabled: false From 26aa0582cff321a521169bd2157c63afffbc126e Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Sun, 7 Apr 2024 21:39:51 +0200 Subject: [PATCH 47/62] Release v1.4.0 --- CHANGES.md | 5 ++++- lib/sidekiq/throttled/version.rb | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index dbe9ff35..130c2701 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [1.4.0] - 2024-04-07 + ### Fixed - Correctly unwrap `ActiveJob` arguments: @@ -111,7 +113,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Remove queue exclusion from fetcher pon throttled job -[unreleased]: https://github.com/ixti/sidekiq-throttled/compare/v1.3.0...main +[unreleased]: https://github.com/ixti/sidekiq-throttled/compare/v1.4.0...main +[1.4.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.3.0...v1.4.0 [1.3.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.2.0...v1.3.0 [1.2.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.1.0...v1.2.0 [1.1.0]: https://github.com/ixti/sidekiq-throttled/compare/v1.0.1...v1.1.0 diff --git a/lib/sidekiq/throttled/version.rb b/lib/sidekiq/throttled/version.rb index 6a0514c2..8500452c 100644 --- a/lib/sidekiq/throttled/version.rb +++ b/lib/sidekiq/throttled/version.rb @@ -3,6 +3,6 @@ module Sidekiq module Throttled # Gem version - VERSION = "1.3.0" + VERSION = "1.4.0" end end From f50baabed910c370ad79c5b89110b9f1669bba65 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Sat, 27 Jan 2024 00:13:04 +0100 Subject: [PATCH 48/62] chore: Drop Sidekiq < 7.0.0 support --- Appraisals | 6 ------ CHANGES.md | 4 ++++ README.adoc | 1 - spec/spec_helper.rb | 2 +- spec/support/sidekiq.rb | 28 ++-------------------------- 5 files changed, 7 insertions(+), 34 deletions(-) diff --git a/Appraisals b/Appraisals index f16caa5c..4e63feb7 100644 --- a/Appraisals +++ b/Appraisals @@ -1,11 +1,5 @@ # frozen_string_literal: true -appraise "sidekiq-6.5.x" do - group :test do - gem "sidekiq", "~> 6.5.0" - end -end - appraise "sidekiq-7.0.x" do group :test do gem "sidekiq", "~> 7.0.0" diff --git a/CHANGES.md b/CHANGES.md index 130c2701..81595d6e 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Removed + +- Drop Sidekiq < 7 support + ## [1.4.0] - 2024-04-07 ### Fixed diff --git a/README.adoc b/README.adoc index c2d3202d..14f82192 100644 --- a/README.adoc +++ b/README.adoc @@ -290,7 +290,6 @@ dropped. This library aims to support and work with following Sidekiq versions: -* Sidekiq 6.5.x * Sidekiq 7.0.x * Sidekiq 7.1.x * Sidekiq 7.2.x diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 167bbe57..fd069beb 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -25,7 +25,7 @@ end # Sidekiq-Pro related specs require license set in Bundler - unless Bundler.settings["gems.contribsys.com"]&.include?(":") && SIDEKIQ7 + unless Bundler.settings["gems.contribsys.com"]&.include?(":") config.define_derived_metadata(sidekiq_pro: true) do |metadata| metadata[:skip] = "Sidekiq::Pro license not found or not supported" end diff --git a/spec/support/sidekiq.rb b/spec/support/sidekiq.rb index f29f217d..e4a19ecd 100644 --- a/spec/support/sidekiq.rb +++ b/spec/support/sidekiq.rb @@ -17,7 +17,6 @@ $TESTING = true # rubocop:disable Style/GlobalVars REDIS_URL = ENV.fetch("REDIS_URL", "redis://localhost:6379") -SIDEKIQ7 = Gem::Version.new("7.0.0") <= Gem::Version.new(Sidekiq::VERSION) module SidekiqThrottledHelper def new_sidekiq_config @@ -31,24 +30,8 @@ def new_sidekiq_config cfg end - def reset_redis! - if SIDEKIQ7 - reset_redis_v7! - else - reset_redis_v6! - end - end - - def reset_redis_v6! - Sidekiq.redis do |conn| - conn.flushdb - conn.script("flush") - end - end - - # Resets Sidekiq config between tests like mperham does in Sidekiq tests: # https://github.com/sidekiq/sidekiq/blob/7df28434f03fa1111e9e2834271c020205369f94/test/helper.rb#L30 - def reset_redis_v7! + def reset_redis! if Sidekiq.default_configuration.instance_variable_defined?(:@redis) existing_pool = Sidekiq.default_configuration.instance_variable_get(:@redis) existing_pool&.shutdown(&:close) @@ -106,15 +89,8 @@ def output end end -if SIDEKIQ7 - Sidekiq.configure_server do |config| - config.queues = %i[default] - end -else - Sidekiq[:queues] = %i[default] -end - Sidekiq.configure_server do |config| + config.queues = %i[default] config.redis = { url: REDIS_URL } config.logger = PseudoLogger.instance end From 2bc1bb41e5fdd31ceb9ac4d6fc249e2347ca8d43 Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Sat, 15 Jun 2024 18:35:55 +0200 Subject: [PATCH 49/62] chore: Remove deprecated setup! method --- CHANGES.md | 1 + lib/sidekiq/throttled.rb | 12 ------------ spec/lib/sidekiq/throttled_spec.rb | 6 ------ 3 files changed, 1 insertion(+), 18 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 81595d6e..e5a2867a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Removed - Drop Sidekiq < 7 support +- Remove deprecated `Sidekiq::Throttled.setup!` ## [1.4.0] - 2024-04-07 diff --git a/lib/sidekiq/throttled.rb b/lib/sidekiq/throttled.rb index 51cc6fd0..6cefcdb8 100644 --- a/lib/sidekiq/throttled.rb +++ b/lib/sidekiq/throttled.rb @@ -87,18 +87,6 @@ def throttled?(message) rescue StandardError false end - - # @deprecated Will be removed in 2.0.0 - def setup! - warn "Sidekiq::Throttled.setup! was deprecated" - - Sidekiq.configure_server do |config| - config.server_middleware do |chain| - chain.remove(Sidekiq::Throttled::Middlewares::Server) - chain.add(Sidekiq::Throttled::Middlewares::Server) - end - end - end end end diff --git a/spec/lib/sidekiq/throttled_spec.rb b/spec/lib/sidekiq/throttled_spec.rb index a3413514..b8f6ad9a 100644 --- a/spec/lib/sidekiq/throttled_spec.rb +++ b/spec/lib/sidekiq/throttled_spec.rb @@ -19,12 +19,6 @@ expect(Sidekiq::BasicFetch).to include(Sidekiq::Throttled::Patches::BasicFetch) end - describe ".setup!" do - it "shows deprecation warning" do - expect { described_class.setup! }.to output(%r{deprecated}).to_stderr - end - end - describe ".throttled?" do it "tolerates invalid JSON message" do expect(described_class.throttled?("][")).to be false From 1c0ecaff37af7aea34e8b21c99fa3cf84500e1ef Mon Sep 17 00:00:00 2001 From: Alexey Zapparov Date: Sat, 15 Jun 2024 18:54:42 +0200 Subject: [PATCH 50/62] lint: Remove deprecated cop department --- rubocop/rspec.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/rubocop/rspec.yml b/rubocop/rspec.yml index 0d754ef5..e9df608b 100644 --- a/rubocop/rspec.yml +++ b/rubocop/rspec.yml @@ -12,6 +12,3 @@ RSpec/MultipleExpectations: RSpec/NamedSubject: Enabled: false - -RSpecRails: - Enabled: false From 39f3f788409e490b97ee9f3a81d3a1aba559fa70 Mon Sep 17 00:00:00 2001 From: Nathan Woodhull Date: Tue, 23 Jul 2024 12:18:46 -0400 Subject: [PATCH 51/62] Update README.adoc (#191) Add a note explaining how to load the throttled dashboard. --------- Signed-off-by: Nathan Woodhull Signed-off-by: Alexey Zapparov Co-authored-by: Alexey Zapparov --- README.adoc | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/README.adoc b/README.adoc index 14f82192..977516a7 100644 --- a/README.adoc +++ b/README.adoc @@ -31,7 +31,6 @@ Or install it yourself as: $ gem install sidekiq-throttled - == Usage Add somewhere in your app's bootstrap (e.g. `config/initializers/sidekiq.rb` if @@ -81,6 +80,17 @@ end ---- +=== Web UI + +To add a Throttled tab to your sidekiq web dashboard, require it durring your +application initialization. + +[source,ruby] +---- +require "sidekiq/throttled/web" +---- + + === Configuration [source,ruby] From f1a86f12bf9660e5515620175f2633510fe4d4e7 Mon Sep 17 00:00:00 2001 From: Bruno Degomme <107565175+bdegomme@users.noreply.github.com> Date: Thu, 7 Nov 2024 00:42:32 +0100 Subject: [PATCH 52/62] fix: Improve cooldown default values (#195) See https://github.com/ixti/sidekiq-throttled/issues/188 I think the default cooldown parameters are poorly chosen, and cause a lot of issues to people starting out with this gem (me included). The most important part is to update the README to insist on how critical those parameters are. And I also think a cooldown_period of 1 and cooldown_threshold of 100 are more reasonable defaults. --------- Co-authored-by: Alexey Zapparov --- CHANGES.md | 6 ++++++ README.adoc | 19 +++++++++++++++---- lib/sidekiq/throttled/config.rb | 4 ++-- sidekiq-throttled.gemspec | 6 +++--- spec/lib/sidekiq/throttled/config_spec.rb | 4 ++-- spec/lib/sidekiq/throttled/registry_spec.rb | 2 +- 6 files changed, 29 insertions(+), 12 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index e5a2867a..276aa169 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Change default cooldown period to `1.0` (was `2.0`), + and cooldown threshold to `100` (was `1`) + [#195](https://github.com/ixti/sidekiq-throttled/pull/195). + ### Removed - Drop Sidekiq < 7 support diff --git a/README.adoc b/README.adoc index 977516a7..b2a1653c 100644 --- a/README.adoc +++ b/README.adoc @@ -99,16 +99,27 @@ Sidekiq::Throttled.configure do |config| # Period in seconds to exclude queue from polling in case it returned # {config.cooldown_threshold} amount of throttled jobs in a row. Set # this value to `nil` to disable cooldown manager completely. - # Default: 2.0 - config.cooldown_period = 2.0 + # Default: 1.0 + config.cooldown_period = 1.0 # Exclude queue from polling after it returned given amount of throttled # jobs in a row. - # Default: 1 (cooldown after first throttled job) - config.cooldown_threshold = 1 + # Default: 100 (cooldown after hundredth throttled job in a row) + config.cooldown_threshold = 100 end ---- +[WARNING] +.Cooldown Settings +==== +If a queue contains a thousand jobs in a row that will be throttled, +the cooldown will kick-in 10 times in a row, meaning it will take 10 seconds +before all those jobs are put back at the end of the queue and you actually +start processing other jobs. + +You may want to adjust the cooldown_threshold and cooldown_period, +keeping in mind that this will also impact the load on your Redis server. +==== ==== Middleware(s) diff --git a/lib/sidekiq/throttled/config.rb b/lib/sidekiq/throttled/config.rb index d75a1195..2eadcfea 100644 --- a/lib/sidekiq/throttled/config.rb +++ b/lib/sidekiq/throttled/config.rb @@ -20,8 +20,8 @@ class Config attr_reader :cooldown_threshold def initialize - @cooldown_period = 2.0 - @cooldown_threshold = 1 + @cooldown_period = 1.0 + @cooldown_threshold = 100 end # @!attribute [w] cooldown_period diff --git a/sidekiq-throttled.gemspec b/sidekiq-throttled.gemspec index 34677fd6..2bdbaa63 100644 --- a/sidekiq-throttled.gemspec +++ b/sidekiq-throttled.gemspec @@ -32,7 +32,7 @@ Gem::Specification.new do |spec| spec.required_ruby_version = ">= 2.7" - spec.add_runtime_dependency "concurrent-ruby", ">= 1.2.0" - spec.add_runtime_dependency "redis-prescription", "~> 2.2" - spec.add_runtime_dependency "sidekiq", ">= 6.5" + spec.add_dependency "concurrent-ruby", ">= 1.2.0" + spec.add_dependency "redis-prescription", "~> 2.2" + spec.add_dependency "sidekiq", ">= 6.5" end diff --git a/spec/lib/sidekiq/throttled/config_spec.rb b/spec/lib/sidekiq/throttled/config_spec.rb index ef6af48d..f10ab700 100644 --- a/spec/lib/sidekiq/throttled/config_spec.rb +++ b/spec/lib/sidekiq/throttled/config_spec.rb @@ -8,7 +8,7 @@ describe "#cooldown_period" do subject { config.cooldown_period } - it { is_expected.to eq 2.0 } + it { is_expected.to eq 1.0 } end describe "#cooldown_period=" do @@ -36,7 +36,7 @@ describe "#cooldown_threshold" do subject { config.cooldown_threshold } - it { is_expected.to eq 1 } + it { is_expected.to eq 100 } end describe "#cooldown_threshold=" do diff --git a/spec/lib/sidekiq/throttled/registry_spec.rb b/spec/lib/sidekiq/throttled/registry_spec.rb index 84041418..e76b1c0d 100644 --- a/spec/lib/sidekiq/throttled/registry_spec.rb +++ b/spec/lib/sidekiq/throttled/registry_spec.rb @@ -111,7 +111,7 @@ def stub_class(name, *parent, &block) describe ".each_with_static_keys" do before do described_class.add("foo", **threshold) - described_class.add("bar", **threshold.merge(key_suffix: ->(i) { i })) + described_class.add("bar", **threshold, key_suffix: ->(i) { i }) end it "yields once for each strategy without dynamic key suffixes" do From bca3912d8a205f1222e6646e9673f7cfc9158a01 Mon Sep 17 00:00:00 2001 From: Olle Jonsson Date: Thu, 7 Nov 2024 00:48:02 +0100 Subject: [PATCH 53/62] chore: Drop exe/ configuration from gemspec (#193) This gem exposes no executables. Signed-off-by: Olle Jonsson --- sidekiq-throttled.gemspec | 2 -- 1 file changed, 2 deletions(-) diff --git a/sidekiq-throttled.gemspec b/sidekiq-throttled.gemspec index 2bdbaa63..7980c359 100644 --- a/sidekiq-throttled.gemspec +++ b/sidekiq-throttled.gemspec @@ -26,8 +26,6 @@ Gem::Specification.new do |spec| end end - spec.bindir = "exe" - spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) } spec.require_paths = ["lib"] spec.required_ruby_version = ">= 2.7" From 93335603533c3a7f080b4836cd945560fdeedd55 Mon Sep 17 00:00:00 2001 From: anero Date: Fri, 8 Nov 2024 12:45:57 -0300 Subject: [PATCH 54/62] Update code to work with latest version on main --- lib/sidekiq/throttled.rb | 5 +++++ lib/sidekiq/throttled/config.rb | 9 +++++++-- lib/sidekiq/throttled/job.rb | 4 ++-- spec/lib/sidekiq/throttled/job_spec.rb | 10 ++++++++-- 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/lib/sidekiq/throttled.rb b/lib/sidekiq/throttled.rb index e2db5b46..67c9016e 100644 --- a/lib/sidekiq/throttled.rb +++ b/lib/sidekiq/throttled.rb @@ -54,6 +54,11 @@ class << self # @return [Cooldown, nil] attr_reader :cooldown + # @api internal + # + # @return [Config, nil] + attr_reader :config + # @example # Sidekiq::Throttled.configure do |config| # config.cooldown_period = nil # Disable queues cooldown manager diff --git a/lib/sidekiq/throttled/config.rb b/lib/sidekiq/throttled/config.rb index e0c9c51e..67213e17 100644 --- a/lib/sidekiq/throttled/config.rb +++ b/lib/sidekiq/throttled/config.rb @@ -30,8 +30,7 @@ class Config attr_reader :default_requeue_options def initialize - @cooldown_period = 2.0 - @cooldown_threshold = 1 + reset! end # @!attribute [w] cooldown_period @@ -56,6 +55,12 @@ def default_requeue_options=(options) @default_requeue_options = options.merge({ with: requeue_with }) end + + def reset! + @cooldown_period = 2.0 + @cooldown_threshold = 1 + @default_requeue_options = { with: :enqueue } + end end end end diff --git a/lib/sidekiq/throttled/job.rb b/lib/sidekiq/throttled/job.rb index b2dac4aa..57fcf9e1 100644 --- a/lib/sidekiq/throttled/job.rb +++ b/lib/sidekiq/throttled/job.rb @@ -33,7 +33,7 @@ module Job # # @private def self.included(base) - worker.sidekiq_class_attribute :sidekiq_throttled_requeue_options + base.sidekiq_class_attribute :sidekiq_throttled_requeue_options base.extend(ClassMethods) end @@ -91,7 +91,7 @@ module ClassMethods # @see Registry.add # @return [void] def sidekiq_throttle(**kwargs) - requeue_options = Throttled.configuration.default_requeue_options.merge(kwargs.delete(:requeue) || {}) + requeue_options = Throttled.config.default_requeue_options.merge(kwargs.delete(:requeue) || {}) unless VALID_VALUES_FOR_REQUEUE_WITH.include?(requeue_options[:with]) raise ArgumentError, "requeue: #{requeue_options[:with]} is not a valid value for :with" end diff --git a/spec/lib/sidekiq/throttled/job_spec.rb b/spec/lib/sidekiq/throttled/job_spec.rb index 9974c21e..b6bef017 100644 --- a/spec/lib/sidekiq/throttled/job_spec.rb +++ b/spec/lib/sidekiq/throttled/job_spec.rb @@ -50,9 +50,15 @@ end context "when default_requeue_options are set" do - before { Sidekiq::Throttled.configuration.default_requeue_options = { with: :schedule } } + before do + Sidekiq::Throttled.configure do |config| + config.default_requeue_options = { with: :schedule } + end + end - after { Sidekiq::Throttled.configuration.reset! } + after do + Sidekiq::Throttled.configure(&:reset!) + end it "uses the default when not overridden" do expect(Sidekiq::Throttled::Registry) From d587010564eed0b0cb5bf407bd292687f9a9dd96 Mon Sep 17 00:00:00 2001 From: anero Date: Fri, 8 Nov 2024 13:13:45 -0300 Subject: [PATCH 55/62] Specs for 'with' option value validation --- spec/lib/sidekiq/throttled/job_spec.rb | 6 ++++++ spec/lib/sidekiq/throttled/strategy_spec.rb | 9 +++++++++ 2 files changed, 15 insertions(+) diff --git a/spec/lib/sidekiq/throttled/job_spec.rb b/spec/lib/sidekiq/throttled/job_spec.rb index b6bef017..9b6772a0 100644 --- a/spec/lib/sidekiq/throttled/job_spec.rb +++ b/spec/lib/sidekiq/throttled/job_spec.rb @@ -49,6 +49,12 @@ expect(working_class.sidekiq_throttled_requeue_options).to eq({ to: :other_queue, with: :schedule }) end + it "raises an error when :with is not a valid value" do + expect do + working_class.sidekiq_throttle(foo: :bar, requeue: { with: :invalid_with_value }) + end.to raise_error(ArgumentError, "requeue: invalid_with_value is not a valid value for :with") + end + context "when default_requeue_options are set" do before do Sidekiq::Throttled.configure do |config| diff --git a/spec/lib/sidekiq/throttled/strategy_spec.rb b/spec/lib/sidekiq/throttled/strategy_spec.rb index 57f3ceea..5ced1e32 100644 --- a/spec/lib/sidekiq/throttled/strategy_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy_spec.rb @@ -397,6 +397,15 @@ def scheduled_redis_item_and_score end end end + + describe "with an invalid :with parameter" do + let(:options) { threshold } + + it "raises an error when :with is not a valid value" do + expect { subject.requeue_throttled(work, with: :invalid_with_value) } + .to raise_error(RuntimeError, "unrecognized :with option invalid_with_value") + end + end end describe "#reset!" do From 401b44dbea371ef9a19cd270d1e569b180734169 Mon Sep 17 00:00:00 2001 From: anero Date: Fri, 8 Nov 2024 13:38:24 -0300 Subject: [PATCH 56/62] Remove obsolete method for requeueing job on Sidekiq Pro (done from Strategy#requeue_throttled instead) --- lib/sidekiq/throttled/patches/super_fetch.rb | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/lib/sidekiq/throttled/patches/super_fetch.rb b/lib/sidekiq/throttled/patches/super_fetch.rb index c1099cd8..d1d33a5f 100644 --- a/lib/sidekiq/throttled/patches/super_fetch.rb +++ b/lib/sidekiq/throttled/patches/super_fetch.rb @@ -14,19 +14,6 @@ def self.prepended(base) private - # Calls SuperFetch UnitOfWork's requeue to remove the job from the - # temporary queue and push job back to the head of the queue, so that - # the job won't be tried immediately after it was requeued (in most cases). - # - # @note This is triggered when job is throttled. - # - # @return [void] - def requeue_throttled(work) - # SuperFetch UnitOfWork's requeue will remove it from the temporary - # queue and then requeue it, so no acknowledgement call is needed. - work.requeue - end - # Returns list of non-paused queues to try to fetch jobs from. # # @note It may return an empty array. From c1b99b50573ca7d19111eda11bcd0d1d5314962c Mon Sep 17 00:00:00 2001 From: Diego Marcet Date: Fri, 8 Nov 2024 16:31:26 -0300 Subject: [PATCH 57/62] Skip sidekiq < 7 support Co-authored-by: Alexey Zapparov Signed-off-by: Diego Marcet --- spec/lib/sidekiq/throttled/strategy_spec.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/spec/lib/sidekiq/throttled/strategy_spec.rb b/spec/lib/sidekiq/throttled/strategy_spec.rb index 5ced1e32..ef3fdc5d 100644 --- a/spec/lib/sidekiq/throttled/strategy_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy_spec.rb @@ -208,11 +208,7 @@ def perform(*); end describe "#requeue_throttled" do let(:sidekiq_config) do - if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") - Sidekiq::DEFAULTS - else - Sidekiq::Config.new(queues: %w[default]).default_capsule - end + Sidekiq::Config.new(queues: %w[default]).default_capsule end let!(:work) do From a709e3da952b947a3f9a69516e664949d4bb4693 Mon Sep 17 00:00:00 2001 From: anero Date: Fri, 8 Nov 2024 16:30:21 -0300 Subject: [PATCH 58/62] Use stub_job_class for defining stubbed job class --- spec/lib/sidekiq/throttled/strategy_spec.rb | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/spec/lib/sidekiq/throttled/strategy_spec.rb b/spec/lib/sidekiq/throttled/strategy_spec.rb index ef3fdc5d..297cb251 100644 --- a/spec/lib/sidekiq/throttled/strategy_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy_spec.rb @@ -1,12 +1,5 @@ # frozen_string_literal: true -class ThrottledTestJob - include Sidekiq::Job - include Sidekiq::Throttled::Job - - def perform(*); end -end - RSpec.describe Sidekiq::Throttled::Strategy do subject(:strategy) { described_class.new(:foo, **options) } @@ -14,6 +7,10 @@ def perform(*); end let(:concurrency) { { concurrency: { limit: 7 } } } let(:ten_seconds_ago) { Time.now - 10 } + before do + stub_job_class("ThrottledTestJob") + end + describe ".new" do it "fails if neither :threshold nor :concurrency given" do expect { described_class.new(:foo) }.to raise_error ArgumentError From b989300b6d53854fa23ff15e9ce6a880438a7293 Mon Sep 17 00:00:00 2001 From: Mauricio Novelo <5916364+mnovelo@users.noreply.github.com> Date: Sat, 9 Nov 2024 12:15:13 -0500 Subject: [PATCH 59/62] Update requeue strategy to work with SuperFetch --- lib/sidekiq/throttled/strategy.rb | 55 ++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 11 deletions(-) diff --git a/lib/sidekiq/throttled/strategy.rb b/lib/sidekiq/throttled/strategy.rb index b5d6e810..f2fc8e1a 100644 --- a/lib/sidekiq/throttled/strategy.rb +++ b/lib/sidekiq/throttled/strategy.rb @@ -84,18 +84,14 @@ def requeue_throttled(work, with:, to: nil) # rubocop:disable Metrics/MethodLeng # Resolve :with and :to arguments, calling them if they are Procs job_args = JSON.parse(work.job)["args"] requeue_with = with.respond_to?(:call) ? with.call(*job_args) : with - requeue_to = to.respond_to?(:call) ? to.call(*job_args) : to + target_queue = calc_target_queue(work, to) case requeue_with when :enqueue - # Push the job back to the head of the queue. - target_list = requeue_to.nil? ? work.queue : "queue:#{requeue_to}" - - # This is the same operation Sidekiq performs upon `Sidekiq::Worker.perform_async` call. - Sidekiq.redis { |conn| conn.lpush(target_list, work.job) } + re_enqueue_throttled(work, target_queue) when :schedule # Find out when we will next be able to execute this job, and reschedule for then. - reschedule_throttled(work, requeue_to: requeue_to) + reschedule_throttled(work, requeue_to: target_queue) else raise "unrecognized :with option #{with}" end @@ -107,7 +103,7 @@ def finalize!(jid, *job_args) @concurrency&.finalize!(jid, *job_args) end - # Resets count of jobs of all avaliable strategies + # Resets count of jobs of all available strategies # @return [void] def reset! @concurrency&.reset! @@ -116,13 +112,50 @@ def reset! private - def reschedule_throttled(work, requeue_to: nil) + def calc_target_queue(work, to) + target = case to + when Proc, Method + to.call(*JSON.parse(work.job)["args"]) + when NilClass + work.queue + when String, Symbol + to.to_s + else + raise ArgumentError, "Invalid argument for `to`" + end + + target = work.queue if target.nil? || target.empty? + + target.start_with?("queue:") ? target : "queue:#{target}" + end + + # Push the job back to the head of the queue. + def re_enqueue_throttled(work, requeue_to) + case work + when Sidekiq::Pro::SuperFetch::UnitOfWork + # Calls SuperFetch UnitOfWork's requeue to remove the job from the + # temporary queue and push job back to the head of the target queue, so that + # the job won't be tried immediately after it was requeued (in most cases). + work.queue = requeue_to if requeue_to + work.requeue + else + # This is the same operation Sidekiq performs upon `Sidekiq::Worker.perform_async` call. + Sidekiq.redis { |conn| conn.lpush(requeue_to, work.job) } + end + end + + def reschedule_throttled(work, requeue_to) message = JSON.parse(work.job) job_class = message.fetch("wrapped") { message.fetch("class") { return false } } job_args = message["args"] - target_queue = requeue_to.nil? ? work.queue : "queue:#{requeue_to}" - Sidekiq::Client.enqueue_to_in(target_queue, retry_in(work), Object.const_get(job_class), *job_args) + # Re-enqueue the job to the target queue at another time as a NEW unit of work + # AND THEN mark this work as done, so SuperFetch doesn't think this instance is orphaned + # Technically, the job could processed twice if the process dies between the two lines, + # but your job should be idempotent anyway, right? + # The job running twice was already a risk with SuperFetch anyway and this doesn't really increase that risk. + Sidekiq::Client.enqueue_to_in(requeue_to, retry_in(work), Object.const_get(job_class), *job_args) + work.acknowledge end def retry_in(work) From 11c6d985bd181523a0824001a7d59eb6ddd069ac Mon Sep 17 00:00:00 2001 From: anero Date: Tue, 12 Nov 2024 16:08:12 -0300 Subject: [PATCH 60/62] Don't reference Sidekiq::Pro::SuperFetch::UnitOfWork class directly in case the gem is not installed Also included: fix rubocop offenses --- lib/sidekiq/throttled/strategy.rb | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/sidekiq/throttled/strategy.rb b/lib/sidekiq/throttled/strategy.rb index f2fc8e1a..4feb7404 100644 --- a/lib/sidekiq/throttled/strategy.rb +++ b/lib/sidekiq/throttled/strategy.rb @@ -112,17 +112,17 @@ def reset! private - def calc_target_queue(work, to) + def calc_target_queue(work, to) # rubocop:disable Metrics/MethodLength target = case to - when Proc, Method - to.call(*JSON.parse(work.job)["args"]) - when NilClass - work.queue - when String, Symbol - to.to_s - else - raise ArgumentError, "Invalid argument for `to`" - end + when Proc, Method + to.call(*JSON.parse(work.job)["args"]) + when NilClass + work.queue + when String, Symbol + to.to_s + else + raise ArgumentError, "Invalid argument for `to`" + end target = work.queue if target.nil? || target.empty? @@ -131,8 +131,8 @@ def calc_target_queue(work, to) # Push the job back to the head of the queue. def re_enqueue_throttled(work, requeue_to) - case work - when Sidekiq::Pro::SuperFetch::UnitOfWork + case work.class.to_s + when "Sidekiq::Pro::SuperFetch::UnitOfWork" # Calls SuperFetch UnitOfWork's requeue to remove the job from the # temporary queue and push job back to the head of the target queue, so that # the job won't be tried immediately after it was requeued (in most cases). @@ -144,7 +144,7 @@ def re_enqueue_throttled(work, requeue_to) end end - def reschedule_throttled(work, requeue_to) + def reschedule_throttled(work, requeue_to:) message = JSON.parse(work.job) job_class = message.fetch("wrapped") { message.fetch("class") { return false } } job_args = message["args"] From 2ebf63fb838207a184fc8f4f594765fc54d96e83 Mon Sep 17 00:00:00 2001 From: Mauricio Novelo <5916364+mnovelo@users.noreply.github.com> Date: Tue, 12 Nov 2024 23:46:13 -0500 Subject: [PATCH 61/62] Add specs --- lib/sidekiq/throttled/strategy.rb | 8 +- spec/lib/sidekiq/throttled/strategy_spec.rb | 187 ++++++++++++++++++++ 2 files changed, 193 insertions(+), 2 deletions(-) diff --git a/lib/sidekiq/throttled/strategy.rb b/lib/sidekiq/throttled/strategy.rb index 4feb7404..a3681a39 100644 --- a/lib/sidekiq/throttled/strategy.rb +++ b/lib/sidekiq/throttled/strategy.rb @@ -131,7 +131,7 @@ def calc_target_queue(work, to) # rubocop:disable Metrics/MethodLength # Push the job back to the head of the queue. def re_enqueue_throttled(work, requeue_to) - case work.class.to_s + case work.class.name when "Sidekiq::Pro::SuperFetch::UnitOfWork" # Calls SuperFetch UnitOfWork's requeue to remove the job from the # temporary queue and push job back to the head of the target queue, so that @@ -165,7 +165,11 @@ def retry_in(work) # Ask both concurrency and threshold, if relevant, how long minimum until we can retry. # If we get two answers, take the longer one. - interval = [@concurrency&.retry_in(jid, *job_args), @threshold&.retry_in(*job_args)].compact.max + intervals = [@concurrency&.retry_in(jid, *job_args), @threshold&.retry_in(*job_args)].compact + + raise "Cannot compute a valid retry interval" if intervals.empty? + + interval = intervals.max # Add a random amount of jitter, proportional to the length of the minimum retry time. # This helps spread out jobs more evenly and avoid clumps of jobs on the queue. diff --git a/spec/lib/sidekiq/throttled/strategy_spec.rb b/spec/lib/sidekiq/throttled/strategy_spec.rb index 297cb251..df7e2afd 100644 --- a/spec/lib/sidekiq/throttled/strategy_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy_spec.rb @@ -399,6 +399,193 @@ def scheduled_redis_item_and_score .to raise_error(RuntimeError, "unrecognized :with option invalid_with_value") end end + + context "when :with is a Proc returning an invalid value" do + let(:options) { threshold } + + it "raises an error when Proc returns an unrecognized value" do + with_proc = ->(*_) { :invalid_value } + expect { + subject.requeue_throttled(work, with: with_proc) + }.to raise_error(RuntimeError, "unrecognized :with option #{with_proc}") + end + end + + context "when :with Proc raises an exception" do + let(:options) { threshold } + + it "propagates the exception" do + faulty_proc = ->(*) { raise "Proc error" } + expect { + subject.requeue_throttled(work, with: faulty_proc) + }.to raise_error("Proc error") + end + end + + describe "with an invalid :to parameter" do + let(:options) { threshold } + + it "raises an ArgumentError when :to is an invalid type" do + invalid_to_value = 12345 # Integer is an invalid type for `to` + expect { + subject.requeue_throttled(work, with: :enqueue, to: invalid_to_value) + }.to raise_error(ArgumentError, "Invalid argument for `to`") + end + end + + context "when :to resolves to nil or empty string" do + let(:options) { threshold } + + it "defaults to work.queue when :to returns nil" do + to_proc = ->(*_) { nil } + expect(strategy).to receive(:re_enqueue_throttled).with(work, work.queue) + subject.requeue_throttled(work, with: :enqueue, to: to_proc) + end + + it "defaults to work.queue when :to returns an empty string" do + to_proc = ->(*_) { "" } + expect(strategy).to receive(:re_enqueue_throttled).with(work, work.queue) + subject.requeue_throttled(work, with: :enqueue, to: to_proc) + end + end + + context "when :to Proc raises an exception" do + let(:options) { threshold } + + it "propagates the exception" do + faulty_proc = ->(*) { raise "Proc error" } + expect { + subject.requeue_throttled(work, with: :enqueue, to: faulty_proc) + }.to raise_error("Proc error") + end + end + + context "when pushing back to Redis" do + let(:options) { threshold } + + it "calls Sidekiq.redis with correct arguments" do + expect(Sidekiq).to receive(:redis).and_yield(double("Redis Connection", lpush: true)) + subject.send(:re_enqueue_throttled, work, "queue:default") + end + end + + describe "#re_enqueue_throttled" do + let(:options) { threshold } + + context "when using Sidekiq Pro's SuperFetch", :sidekiq_pro do + let!(:work) do + # Sidekiq is FIFO queue, with head on right side of the list, + # meaning jobs below will be stored in 3, 2, 1 order. + ThrottledTestJob.perform_bulk([[1], [2], [3]]) + + # Pop the work off the queue + job = Sidekiq.redis do |conn| + conn.rpop("queue:default") + end + super_fetch_uow = Object.const_get("Sidekiq::Pro::SuperFetch::UnitOfWork") + super_fetch_uow.new("queue:default", job, "local_queue", sidekiq_config) + end + + it "calls work.requeue and updates work.queue if requeue_to is provided" do + expect(work).to receive(:queue=).with("queue:other_queue") + expect(work).to receive(:requeue) + subject.send(:re_enqueue_throttled, work, "queue:other_queue") + end + + it "calls work.requeue without updating work.queue if requeue_to is nil" do + expect(work).not_to receive(:queue=) + expect(work).to receive(:requeue) + subject.send(:re_enqueue_throttled, work, nil) + end + end + + context "when using Sidekiq BasicFetch" do + it "pushes the job back onto the queue using Sidekiq.redis" do + redis_mock = double("Redis Connection") + expect(Sidekiq).to receive(:redis).and_yield(redis_mock) + expect(redis_mock).to receive(:lpush).with("queue:default", work.job) + subject.send(:re_enqueue_throttled, work, "queue:default") + end + end + end + + describe "#reschedule_throttled" do + let(:options) { threshold } + + context "when job_class is missing from work.job" do + before do + invalid_job_data = JSON.parse(work.job).tap { |msg| msg.delete("class"); msg.delete("wrapped") } + allow(work).to receive(:job).and_return(invalid_job_data.to_json) + end + + it "returns false and does not reschedule the job" do + expect(Sidekiq::Client).not_to receive(:enqueue_to_in) + expect(work).not_to receive(:acknowledge) + expect(subject.send(:reschedule_throttled, work, requeue_to: "queue:default")).to be_falsey + end + end + + context "when job_class is present in work.job" do + before do + allow(subject).to receive(:retry_in).and_return(300.0) + end + + it "calls Sidekiq::Client.enqueue_to_in with correct arguments" do + job_args = JSON.parse(work.job)["args"] + expect(Sidekiq::Client).to receive(:enqueue_to_in).with( + "queue:default", + 300.0, + ThrottledTestJob, + *job_args + ) + expect(work).to receive(:acknowledge) + subject.send(:reschedule_throttled, work, requeue_to: "queue:default") + end + end + end + + describe "#retry_in" do + context "when both strategies return nil" do + let(:options) { concurrency.merge(threshold) } + + before do + allow(subject.concurrency).to receive(:retry_in).and_return(nil) + allow(subject.threshold).to receive(:retry_in).and_return(nil) + end + + it "raises an error indicating it cannot compute a valid retry interval" do + expect { + subject.send(:retry_in, work) + }.to raise_error("Cannot compute a valid retry interval") + end + end + + context "when interval is less than or equal to 10 (no jitter)" do + let(:options) { threshold } + + before do + allow(subject.threshold).to receive(:retry_in).and_return(10.0) + allow(subject).to receive(:rand).and_return(2) # Control randomness + end + + it "does not add jitter when interval is 10 or less" do + expect(subject.send(:retry_in, work)).to eq(10.0) + end + end + + context "when interval is greater than 10 (jitter added)" do + let(:options) { threshold } + + before do + allow(subject.threshold).to receive(:retry_in).and_return(100.0) + allow(subject).to receive(:rand).with(20.0).and_return(5.0) # interval / 5 = 20.0 + end + + it "adds jitter when interval is greater than 10" do + expect(subject.send(:retry_in, work)).to eq(105.0) # 100.0 + 5.0 + end + end + end end describe "#reset!" do From 39522bc119aa8742e1a5a01770f422b5c9ed9198 Mon Sep 17 00:00:00 2001 From: Mauricio Novelo <5916364+mnovelo@users.noreply.github.com> Date: Thu, 14 Nov 2024 01:19:09 -0500 Subject: [PATCH 62/62] Fix rubocop warnings --- .../throttled/patches/basic_fetch_spec.rb | 4 +- .../throttled/patches/super_fetch_spec.rb | 4 +- spec/lib/sidekiq/throttled/strategy_spec.rb | 881 ++++++++++++------ spec/support/sidekiq.rb | 5 +- 4 files changed, 621 insertions(+), 273 deletions(-) diff --git a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb index d5fa1ebc..de70974b 100644 --- a/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb +++ b/spec/lib/sidekiq/throttled/patches/basic_fetch_spec.rb @@ -49,7 +49,7 @@ fetch.retrieve_work expect { fetch.retrieve_work } - .to change { enqueued_jobs("default") }.to([["TestJob", 2], ["TestJob", 3]]) + .to change { enqueued_jobs("default") }.to([["TestJob", [2]], ["TestJob", [3]]]) .and(keep_unchanged { enqueued_jobs("critical") }) end @@ -65,7 +65,7 @@ it "updates cooldown queues" do expect { fetch.retrieve_work } - .to change { enqueued_jobs("default") }.to([["TestJob", 2], ["TestJob", 3]]) + .to change { enqueued_jobs("default") }.to([["TestJob", [2]], ["TestJob", [3]]]) .and(change { Sidekiq::Throttled.cooldown.queues }.to(["queue:default"])) end diff --git a/spec/lib/sidekiq/throttled/patches/super_fetch_spec.rb b/spec/lib/sidekiq/throttled/patches/super_fetch_spec.rb index 784add02..4bc7219e 100644 --- a/spec/lib/sidekiq/throttled/patches/super_fetch_spec.rb +++ b/spec/lib/sidekiq/throttled/patches/super_fetch_spec.rb @@ -53,7 +53,7 @@ fetch.retrieve_work expect { fetch.retrieve_work } - .to change { enqueued_jobs(base_queue) }.to([["TestJob", 2], ["TestJob", 3]]) + .to change { enqueued_jobs(base_queue) }.to([["TestJob", [2]], ["TestJob", [3]]]) .and(keep_unchanged { enqueued_jobs(critical_queue) }) end @@ -69,7 +69,7 @@ it "updates cooldown queues" do expect { fetch.retrieve_work } - .to change { enqueued_jobs(base_queue) }.to([["TestJob", 2], ["TestJob", 3]]) + .to change { enqueued_jobs(base_queue) }.to([["TestJob", [2]], ["TestJob", [3]]]) .and(change { Sidekiq::Throttled.cooldown.queues }.to(["queue:#{base_queue}"])) end diff --git a/spec/lib/sidekiq/throttled/strategy_spec.rb b/spec/lib/sidekiq/throttled/strategy_spec.rb index df7e2afd..4c17d497 100644 --- a/spec/lib/sidekiq/throttled/strategy_spec.rb +++ b/spec/lib/sidekiq/throttled/strategy_spec.rb @@ -204,385 +204,732 @@ end describe "#requeue_throttled" do - let(:sidekiq_config) do - Sidekiq::Config.new(queues: %w[default]).default_capsule + def scheduled_redis_item_and_score + Sidekiq.redis do |conn| + # Depending on whether we have redis-client (for Sidekiq 7) or redis-rb (for older Sidekiq), + # zscan takes different arguments + if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") + conn.zscan("schedule", 0).last.first + else + conn.zscan("schedule").first + end + end end - let!(:work) do - # Sidekiq is FIFO queue, with head on right side of the list, - # meaning jobs below will be stored in 3, 2, 1 order. - ThrottledTestJob.perform_bulk([[1], [2], [3]]) + context "when using Sidekiq Pro's SuperFetch", :sidekiq_pro do + let(:sidekiq_config) do + config = Sidekiq::Config.new(queues: %w[default other_queue]) + config.super_fetch! + config + end + let(:fetcher) { sidekiq_config.default_capsule.fetcher } + + let(:work) { fetcher.retrieve_work } - # Pop the work off the queue - job = Sidekiq.redis do |conn| - conn.rpop("queue:default") + before do + pre_existing_job = fetcher.retrieve_work + raise "Found pre-existing job: #{pre_existing_job.inspect}" if pre_existing_job + + # Sidekiq is FIFO queue, with head on right side of the list, + # meaning jobs below will be stored in 3, 2, 1 order. + ThrottledTestJob.perform_bulk([[1], [2], [3]]) + work end - Sidekiq::BasicFetch::UnitOfWork.new("queue:default", job, sidekiq_config) - end - describe "with parameter with: :enqueue" do - let(:options) { threshold } + describe "with parameter with: :enqueue" do + let(:options) { threshold } - def enqueued_jobs(queue) - Sidekiq.redis do |conn| - conn.lrange("queue:#{queue}", 0, -1).map do |job| - JSON.parse(job).then do |payload| - [payload["class"], payload["args"]] - end - end + it "puts the job back on the queue" do + # Ensure that the job was removed from default queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + # And added to the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to eq([["ThrottledTestJob", [1]]]) + expect(enqueued_jobs("other_queue")).to be_empty + + # Requeue the work + subject.requeue_throttled(work, with: :enqueue) + + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], + ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty + + # Ensure that the job is no longer in the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to be_empty end - end - it "puts the job back on the queue" do - expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) - expect(enqueued_jobs("other_queue")).to be_empty + it "puts the job back on a different queue when specified" do + # Ensure that the job was removed from default queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + # And added to the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to eq([["ThrottledTestJob", [1]]]) + expect(enqueued_jobs("other_queue")).to be_empty - # Requeue the work - subject.requeue_throttled(work, with: :enqueue) + # Requeue the work + subject.requeue_throttled(work, with: :enqueue, to: :other_queue) - # See that it is now on the end of the queue - expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], - ["ThrottledTestJob", [2]]]) - expect(enqueued_jobs("other_queue")).to be_empty - end + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to eq([["ThrottledTestJob", [1]]]) - it "puts the job back on a different queue when specified" do - expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) - expect(enqueued_jobs("other_queue")).to be_empty + # Ensure that the job is no longer in the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to be_empty + end - # Requeue the work - subject.requeue_throttled(work, with: :enqueue, to: :other_queue) + it "accepts a Proc for :with argument" do + # Ensure that the job was removed from default queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + # And added to the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to eq([["ThrottledTestJob", [1]]]) + expect(enqueued_jobs("other_queue")).to be_empty - # See that it is now on the end of the queue - expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) - expect(enqueued_jobs("other_queue")).to eq([["ThrottledTestJob", [1]]]) - end + # Requeue the work + subject.requeue_throttled(work, with: ->(_arg) { :enqueue }) - it "accepts a Proc for :with argument" do - expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) - expect(enqueued_jobs("other_queue")).to be_empty + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], + ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty - # Requeue the work - subject.requeue_throttled(work, with: ->(_arg) { :enqueue }) + # Ensure that the job is no longer in the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to be_empty + end - # See that it is now on the end of the queue - expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], - ["ThrottledTestJob", [2]]]) - expect(enqueued_jobs("other_queue")).to be_empty - end + it "accepts a Proc for :to argument" do + # Ensure that the job was removed from default queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + # And added to the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to eq([["ThrottledTestJob", [1]]]) + expect(enqueued_jobs("other_queue")).to be_empty - it "accepts a Proc for :to argument" do - expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) - expect(enqueued_jobs("other_queue")).to be_empty + # Requeue the work + subject.requeue_throttled(work, with: :enqueue, to: ->(_arg) { :other_queue }) - # Requeue the work - subject.requeue_throttled(work, with: :enqueue, to: ->(_arg) { :other_queue }) + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to eq([["ThrottledTestJob", [1]]]) - # See that it is now on the end of the queue - expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) - expect(enqueued_jobs("other_queue")).to eq([["ThrottledTestJob", [1]]]) - end - end + # Ensure that the job is no longer in the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to be_empty + end - describe "with parameter with: :schedule" do - def scheduled_redis_item_and_score - Sidekiq.redis do |conn| - # Depending on whether we have redis-client (for Sidekiq 7) or redis-rb (for older Sidekiq), - # zscan takes different arguments - if Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("7.0.0") - conn.zscan("schedule", 0).last.first - else - conn.zscan("schedule").first + describe "with an invalid :to parameter" do + let(:options) { threshold } + + it "raises an ArgumentError when :to is an invalid type" do + invalid_to_value = 12_345 # Integer is an invalid type for `to` + expect do + subject.requeue_throttled(work, with: :enqueue, to: invalid_to_value) + end.to raise_error(ArgumentError, "Invalid argument for `to`") + end + end + + context "when :to Proc raises an exception" do + let(:options) { threshold } + + it "propagates the exception" do + faulty_proc = ->(*) { raise "Proc error" } + expect do + subject.requeue_throttled(work, with: :enqueue, to: faulty_proc) + end.to raise_error("Proc error") end end end - context "when threshold constraints given" do - let(:options) { threshold } + describe "with parameter with: :schedule" do + context "when threshold constraints given" do + let(:options) { threshold } - before do - allow(subject.threshold).to receive(:retry_in).and_return(300.0) + before do + allow(subject.threshold).to receive(:retry_in).and_return(300.0) + end + + it "reschedules for when the threshold strategy says to, plus some jitter" do + # Ensure that the job was removed from default queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + # And added to the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to eq([["ThrottledTestJob", [1]]]) + + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: :schedule) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:default") + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + + # Ensure that the job is no longer in the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to be_empty + end + + it "reschedules for a different queue if specified" do + # Ensure that the job was removed from default queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + # And added to the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to eq([["ThrottledTestJob", [1]]]) + + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: :schedule, to: :other_queue) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:other_queue") + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + + # Ensure that the job is no longer in the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to be_empty + end + + it "accepts a Proc for :with argument" do + # Ensure that the job was removed from default queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + # And added to the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to eq([["ThrottledTestJob", [1]]]) + + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: ->(_arg) { :schedule }) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:default") + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + + # Ensure that the job is no longer in the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to be_empty + end + + it "accepts a Proc for :to argument" do + # Ensure that the job was removed from default queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + # And added to the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to eq([["ThrottledTestJob", [1]]]) + + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: :schedule, to: ->(_arg) { :other_queue }) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:other_queue") + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + + # Ensure that the job is no longer in the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to be_empty + end end - it "reschedules for when the threshold strategy says to, plus some jitter" do - # Requeue the work, see that it ends up in 'schedule' - expect do - subject.requeue_throttled(work, with: :schedule) - end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + context "when concurrency constraints given" do + let(:options) { concurrency } + + before do + allow(subject.concurrency).to receive(:retry_in).and_return(300.0) + end + + it "reschedules for when the concurrency strategy says to, plus some jitter" do + # Ensure that the job was removed from default queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + # And added to the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to eq([["ThrottledTestJob", [1]]]) - item, score = scheduled_redis_item_and_score - expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") - expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: :schedule) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:default") + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + + # Ensure that the job is no longer in the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to be_empty + end end - it "reschedules for a different queue if specified" do - # Requeue the work, see that it ends up in 'schedule' - expect do - subject.requeue_throttled(work, with: :schedule, to: :other_queue) - end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + context "when threshold and concurrency constraints given" do + let(:options) { threshold.merge concurrency } + + before do + allow(subject.concurrency).to receive(:retry_in).and_return(300.0) + allow(subject.threshold).to receive(:retry_in).and_return(500.0) + end - item, score = scheduled_redis_item_and_score - expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], - "queue" => "queue:other_queue") - expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + it "reschedules for the later of what the two say, plus some jitter" do + # Ensure that the job was removed from default queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + # And added to the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to eq([["ThrottledTestJob", [1]]]) + + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: :schedule) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:default") + expect(score.to_f).to be_within(51.0).of(Time.now.to_f + 550.0) + + # Ensure that the job is no longer in the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to be_empty + end end - it "accepts a Proc for :with argument" do - # Requeue the work, see that it ends up in 'schedule' - expect do - subject.requeue_throttled(work, with: ->(_arg) { :schedule }) - end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + describe "with an invalid :to parameter" do + let(:options) { threshold } - item, score = scheduled_redis_item_and_score - expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") - expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + it "raises an ArgumentError when :to is an invalid type" do + invalid_to_value = 12_345 # Integer is an invalid type for `to` + expect do + subject.requeue_throttled(work, with: :schedule, to: invalid_to_value) + end.to raise_error(ArgumentError, "Invalid argument for `to`") + end end - it "accepts a Proc for :to argument" do - # Requeue the work, see that it ends up in 'schedule' - expect do - subject.requeue_throttled(work, with: :schedule, to: ->(_arg) { :other_queue }) - end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + context "when :to Proc raises an exception" do + let(:options) { threshold } - item, score = scheduled_redis_item_and_score - expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], - "queue" => "queue:other_queue") - expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + it "propagates the exception" do + faulty_proc = ->(*) { raise "Proc error" } + expect do + subject.requeue_throttled(work, with: :schedule, to: faulty_proc) + end.to raise_error("Proc error") + end end end - context "when concurrency constraints given" do - let(:options) { concurrency } + describe "with an invalid :with parameter" do + let(:options) { threshold } - before do - allow(subject.concurrency).to receive(:retry_in).and_return(300.0) + it "raises an error when :with is not a valid value" do + expect { subject.requeue_throttled(work, with: :invalid_with_value) } + .to raise_error(RuntimeError, "unrecognized :with option invalid_with_value") end + end + + context "when :with is a Proc returning an invalid value" do + let(:options) { threshold } - it "reschedules for when the concurrency strategy says to, plus some jitter" do - # Requeue the work, see that it ends up in 'schedule' + it "raises an error when Proc returns an unrecognized value" do + with_proc = ->(*_) { :invalid_value } expect do - subject.requeue_throttled(work, with: :schedule) - end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + subject.requeue_throttled(work, with: with_proc) + end.to raise_error(RuntimeError, "unrecognized :with option #{with_proc}") + end + end + + context "when :with Proc raises an exception" do + let(:options) { threshold } - item, score = scheduled_redis_item_and_score - expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") - expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + it "propagates the exception" do + faulty_proc = ->(*) { raise "Proc error" } + expect do + subject.requeue_throttled(work, with: faulty_proc) + end.to raise_error("Proc error") end end - context "when threshold and concurrency constraints given" do - let(:options) { threshold.merge concurrency } + context "when :to resolves to nil or empty string" do + let(:options) { threshold } + + it "defaults to work.queue when :to returns nil" do + to_proc = ->(*_) {} + # Ensure that the job was removed from default queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + # And added to the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to eq([["ThrottledTestJob", [1]]]) + expect(enqueued_jobs("other_queue")).to be_empty - before do - allow(subject.concurrency).to receive(:retry_in).and_return(300.0) - allow(subject.threshold).to receive(:retry_in).and_return(500.0) + # Requeue the work + subject.requeue_throttled(work, with: :enqueue, to: to_proc) + + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], + ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty + + # Ensure that the job is no longer in the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to be_empty end - it "reschedules for the later of what the two say, plus some jitter" do - # Requeue the work, see that it ends up in 'schedule' - expect do - subject.requeue_throttled(work, with: :schedule) - end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + it "defaults to work.queue when :to returns an empty string" do + to_proc = ->(*_) { "" } + # Ensure that the job was removed from default queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + # And added to the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to eq([["ThrottledTestJob", [1]]]) + expect(enqueued_jobs("other_queue")).to be_empty + + # Requeue the work + subject.requeue_throttled(work, with: :enqueue, to: to_proc) - item, score = scheduled_redis_item_and_score - expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], "queue" => "queue:default") - expect(score.to_f).to be_within(51.0).of(Time.now.to_f + 550.0) + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], + ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty + + # Ensure that the job is no longer in the private queue + expect(enqueued_jobs(fetcher.private_queue("default"))).to be_empty end end - end - describe "with an invalid :with parameter" do - let(:options) { threshold } + describe "#reschedule_throttled" do + let(:options) { threshold } + + context "when job_class is missing from work.job" do + before do + invalid_job_data = JSON.parse(work.job).tap do |msg| + msg.delete("class") + msg.delete("wrapped") + end + allow(work).to receive(:job).and_return(invalid_job_data.to_json) + end - it "raises an error when :with is not a valid value" do - expect { subject.requeue_throttled(work, with: :invalid_with_value) } - .to raise_error(RuntimeError, "unrecognized :with option invalid_with_value") + it "returns false and does not reschedule the job" do + expect(Sidekiq::Client).not_to receive(:enqueue_to_in) + expect(work).not_to receive(:acknowledge) + expect(subject.send(:reschedule_throttled, work, requeue_to: "queue:default")).to be_falsey + end + end end - end - context "when :with is a Proc returning an invalid value" do - let(:options) { threshold } + describe "#retry_in" do + context "when both strategies return nil" do + let(:options) { concurrency.merge(threshold) } + + before do + allow(subject.concurrency).to receive(:retry_in).and_return(nil) + allow(subject.threshold).to receive(:retry_in).and_return(nil) + end - it "raises an error when Proc returns an unrecognized value" do - with_proc = ->(*_) { :invalid_value } - expect { - subject.requeue_throttled(work, with: with_proc) - }.to raise_error(RuntimeError, "unrecognized :with option #{with_proc}") + it "raises an error indicating it cannot compute a valid retry interval" do + expect do + subject.send(:retry_in, work) + end.to raise_error("Cannot compute a valid retry interval") + end + end end end - context "when :with Proc raises an exception" do - let(:options) { threshold } - - it "propagates the exception" do - faulty_proc = ->(*) { raise "Proc error" } - expect { - subject.requeue_throttled(work, with: faulty_proc) - }.to raise_error("Proc error") + context "when using Sidekiq BasicFetch" do + let(:sidekiq_config) do + Sidekiq::Config.new(queues: %w[default]) end - end + let(:fetcher) { sidekiq_config.default_capsule.fetcher } - describe "with an invalid :to parameter" do - let(:options) { threshold } + let(:work) { fetcher.retrieve_work } - it "raises an ArgumentError when :to is an invalid type" do - invalid_to_value = 12345 # Integer is an invalid type for `to` - expect { - subject.requeue_throttled(work, with: :enqueue, to: invalid_to_value) - }.to raise_error(ArgumentError, "Invalid argument for `to`") + before do + pre_existing_job = fetcher.retrieve_work + raise "Found pre-existing job: #{pre_existing_job.inspect}" if pre_existing_job + + # Sidekiq is FIFO queue, with head on right side of the list, + # meaning jobs below will be stored in 3, 2, 1 order. + ThrottledTestJob.perform_bulk([[1], [2], [3]]) + work end - end - context "when :to resolves to nil or empty string" do - let(:options) { threshold } + describe "with parameter with: :enqueue" do + let(:options) { threshold } - it "defaults to work.queue when :to returns nil" do - to_proc = ->(*_) { nil } - expect(strategy).to receive(:re_enqueue_throttled).with(work, work.queue) - subject.requeue_throttled(work, with: :enqueue, to: to_proc) - end + it "puts the job back on the queue" do + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty - it "defaults to work.queue when :to returns an empty string" do - to_proc = ->(*_) { "" } - expect(strategy).to receive(:re_enqueue_throttled).with(work, work.queue) - subject.requeue_throttled(work, with: :enqueue, to: to_proc) - end - end + # Requeue the work + subject.requeue_throttled(work, with: :enqueue) - context "when :to Proc raises an exception" do - let(:options) { threshold } + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], + ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty + end - it "propagates the exception" do - faulty_proc = ->(*) { raise "Proc error" } - expect { - subject.requeue_throttled(work, with: :enqueue, to: faulty_proc) - }.to raise_error("Proc error") - end - end + it "puts the job back on a different queue when specified" do + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty - context "when pushing back to Redis" do - let(:options) { threshold } + # Requeue the work + subject.requeue_throttled(work, with: :enqueue, to: :other_queue) - it "calls Sidekiq.redis with correct arguments" do - expect(Sidekiq).to receive(:redis).and_yield(double("Redis Connection", lpush: true)) - subject.send(:re_enqueue_throttled, work, "queue:default") - end - end + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to eq([["ThrottledTestJob", [1]]]) + end - describe "#re_enqueue_throttled" do - let(:options) { threshold } + it "accepts a Proc for :with argument" do + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty - context "when using Sidekiq Pro's SuperFetch", :sidekiq_pro do - let!(:work) do - # Sidekiq is FIFO queue, with head on right side of the list, - # meaning jobs below will be stored in 3, 2, 1 order. - ThrottledTestJob.perform_bulk([[1], [2], [3]]) + # Requeue the work + subject.requeue_throttled(work, with: ->(_arg) { :enqueue }) - # Pop the work off the queue - job = Sidekiq.redis do |conn| - conn.rpop("queue:default") - end - super_fetch_uow = Object.const_get("Sidekiq::Pro::SuperFetch::UnitOfWork") - super_fetch_uow.new("queue:default", job, "local_queue", sidekiq_config) + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], + ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty end - it "calls work.requeue and updates work.queue if requeue_to is provided" do - expect(work).to receive(:queue=).with("queue:other_queue") - expect(work).to receive(:requeue) - subject.send(:re_enqueue_throttled, work, "queue:other_queue") + it "accepts a Proc for :to argument" do + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty + + # Requeue the work + subject.requeue_throttled(work, with: :enqueue, to: ->(_arg) { :other_queue }) + + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to eq([["ThrottledTestJob", [1]]]) end - it "calls work.requeue without updating work.queue if requeue_to is nil" do - expect(work).not_to receive(:queue=) - expect(work).to receive(:requeue) - subject.send(:re_enqueue_throttled, work, nil) + describe "with an invalid :to parameter" do + let(:options) { threshold } + + it "raises an ArgumentError when :to is an invalid type" do + invalid_to_value = 12_345 # Integer is an invalid type for `to` + expect do + subject.requeue_throttled(work, with: :enqueue, to: invalid_to_value) + end.to raise_error(ArgumentError, "Invalid argument for `to`") + end end - end - context "when using Sidekiq BasicFetch" do - it "pushes the job back onto the queue using Sidekiq.redis" do - redis_mock = double("Redis Connection") - expect(Sidekiq).to receive(:redis).and_yield(redis_mock) - expect(redis_mock).to receive(:lpush).with("queue:default", work.job) - subject.send(:re_enqueue_throttled, work, "queue:default") + context "when :to Proc raises an exception" do + let(:options) { threshold } + + it "propagates the exception" do + faulty_proc = ->(*) { raise "Proc error" } + expect do + subject.requeue_throttled(work, with: :enqueue, to: faulty_proc) + end.to raise_error("Proc error") + end end end - end - describe "#reschedule_throttled" do - let(:options) { threshold } + describe "with parameter with: :schedule" do + context "when threshold constraints given" do + let(:options) { threshold } - context "when job_class is missing from work.job" do - before do - invalid_job_data = JSON.parse(work.job).tap { |msg| msg.delete("class"); msg.delete("wrapped") } - allow(work).to receive(:job).and_return(invalid_job_data.to_json) + before do + allow(subject.threshold).to receive(:retry_in).and_return(300.0) + end + + it "reschedules for when the threshold strategy says to, plus some jitter" do + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: :schedule) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:default") + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + end + + it "reschedules for a different queue if specified" do + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: :schedule, to: :other_queue) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:other_queue") + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + end + + it "accepts a Proc for :with argument" do + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: ->(_arg) { :schedule }) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:default") + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + end + + it "accepts a Proc for :to argument" do + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: :schedule, to: ->(_arg) { :other_queue }) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:other_queue") + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + end end - it "returns false and does not reschedule the job" do - expect(Sidekiq::Client).not_to receive(:enqueue_to_in) - expect(work).not_to receive(:acknowledge) - expect(subject.send(:reschedule_throttled, work, requeue_to: "queue:default")).to be_falsey + context "when concurrency constraints given" do + let(:options) { concurrency } + + before do + allow(subject.concurrency).to receive(:retry_in).and_return(300.0) + end + + it "reschedules for when the concurrency strategy says to, plus some jitter" do + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: :schedule) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:default") + expect(score.to_f).to be_within(31.0).of(Time.now.to_f + 330.0) + end end - end - context "when job_class is present in work.job" do - before do - allow(subject).to receive(:retry_in).and_return(300.0) + context "when threshold and concurrency constraints given" do + let(:options) { threshold.merge concurrency } + + before do + allow(subject.concurrency).to receive(:retry_in).and_return(300.0) + allow(subject.threshold).to receive(:retry_in).and_return(500.0) + end + + it "reschedules for the later of what the two say, plus some jitter" do + # Requeue the work, see that it ends up in 'schedule' + expect do + subject.requeue_throttled(work, with: :schedule) + end.to change { Sidekiq.redis { |conn| conn.zcard("schedule") } }.by(1) + + item, score = scheduled_redis_item_and_score + expect(JSON.parse(item)).to include("class" => "ThrottledTestJob", "args" => [1], + "queue" => "queue:default") + expect(score.to_f).to be_within(51.0).of(Time.now.to_f + 550.0) + end + end + + describe "with an invalid :to parameter" do + let(:options) { threshold } + + it "raises an ArgumentError when :to is an invalid type" do + invalid_to_value = 12_345 # Integer is an invalid type for `to` + expect do + subject.requeue_throttled(work, with: :schedule, to: invalid_to_value) + end.to raise_error(ArgumentError, "Invalid argument for `to`") + end end - it "calls Sidekiq::Client.enqueue_to_in with correct arguments" do - job_args = JSON.parse(work.job)["args"] - expect(Sidekiq::Client).to receive(:enqueue_to_in).with( - "queue:default", - 300.0, - ThrottledTestJob, - *job_args - ) - expect(work).to receive(:acknowledge) - subject.send(:reschedule_throttled, work, requeue_to: "queue:default") + context "when :to Proc raises an exception" do + let(:options) { threshold } + + it "propagates the exception" do + faulty_proc = ->(*) { raise "Proc error" } + expect do + subject.requeue_throttled(work, with: :schedule, to: faulty_proc) + end.to raise_error("Proc error") + end end end - end - describe "#retry_in" do - context "when both strategies return nil" do - let(:options) { concurrency.merge(threshold) } + describe "with an invalid :with parameter" do + let(:options) { threshold } - before do - allow(subject.concurrency).to receive(:retry_in).and_return(nil) - allow(subject.threshold).to receive(:retry_in).and_return(nil) + it "raises an error when :with is not a valid value" do + expect { subject.requeue_throttled(work, with: :invalid_with_value) } + .to raise_error(RuntimeError, "unrecognized :with option invalid_with_value") end + end - it "raises an error indicating it cannot compute a valid retry interval" do - expect { - subject.send(:retry_in, work) - }.to raise_error("Cannot compute a valid retry interval") + context "when :with is a Proc returning an invalid value" do + let(:options) { threshold } + + it "raises an error when Proc returns an unrecognized value" do + with_proc = ->(*_) { :invalid_value } + expect do + subject.requeue_throttled(work, with: with_proc) + end.to raise_error(RuntimeError, "unrecognized :with option #{with_proc}") end end - context "when interval is less than or equal to 10 (no jitter)" do + context "when :with Proc raises an exception" do let(:options) { threshold } - before do - allow(subject.threshold).to receive(:retry_in).and_return(10.0) - allow(subject).to receive(:rand).and_return(2) # Control randomness + it "propagates the exception" do + faulty_proc = ->(*) { raise "Proc error" } + expect do + subject.requeue_throttled(work, with: faulty_proc) + end.to raise_error("Proc error") + end + end + + context "when :to resolves to nil or empty string" do + let(:options) { threshold } + + it "defaults to work.queue when :to returns nil" do + to_proc = ->(*_) {} + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty + + # Requeue the work + subject.requeue_throttled(work, with: :enqueue, to: to_proc) + + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], + ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty end - it "does not add jitter when interval is 10 or less" do - expect(subject.send(:retry_in, work)).to eq(10.0) + it "defaults to work.queue when :to returns an empty string" do + to_proc = ->(*_) { "" } + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [3]], ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty + + # Requeue the work + subject.requeue_throttled(work, with: :enqueue, to: to_proc) + + # See that it is now on the end of the queue + expect(enqueued_jobs("default")).to eq([["ThrottledTestJob", [1]], ["ThrottledTestJob", [3]], + ["ThrottledTestJob", [2]]]) + expect(enqueued_jobs("other_queue")).to be_empty end end - context "when interval is greater than 10 (jitter added)" do + describe "#reschedule_throttled" do let(:options) { threshold } - before do - allow(subject.threshold).to receive(:retry_in).and_return(100.0) - allow(subject).to receive(:rand).with(20.0).and_return(5.0) # interval / 5 = 20.0 + context "when job_class is missing from work.job" do + before do + invalid_job_data = JSON.parse(work.job).tap do |msg| + msg.delete("class") + msg.delete("wrapped") + end + allow(work).to receive(:job).and_return(invalid_job_data.to_json) + end + + it "returns false and does not reschedule the job" do + expect(Sidekiq::Client).not_to receive(:enqueue_to_in) + expect(work).not_to receive(:acknowledge) + expect(subject.send(:reschedule_throttled, work, requeue_to: "queue:default")).to be_falsey + end end + end + + describe "#retry_in" do + context "when both strategies return nil" do + let(:options) { concurrency.merge(threshold) } - it "adds jitter when interval is greater than 10" do - expect(subject.send(:retry_in, work)).to eq(105.0) # 100.0 + 5.0 + before do + allow(subject.concurrency).to receive(:retry_in).and_return(nil) + allow(subject.threshold).to receive(:retry_in).and_return(nil) + end + + it "raises an error indicating it cannot compute a valid retry interval" do + expect do + subject.send(:retry_in, work) + end.to raise_error("Cannot compute a valid retry interval") + end end end end diff --git a/spec/support/sidekiq.rb b/spec/support/sidekiq.rb index e4a19ecd..c8f69b47 100644 --- a/spec/support/sidekiq.rb +++ b/spec/support/sidekiq.rb @@ -58,10 +58,11 @@ def perform(*); end end def enqueued_jobs(queue) + q = queue.start_with?("queue:") ? queue : "queue:#{queue}" Sidekiq.redis do |conn| - conn.lrange("queue:#{queue}", 0, -1).map do |job| + conn.lrange(q, 0, -1).map do |job| JSON.parse(job).then do |payload| - [payload["class"], *payload["args"]] + [payload["class"], payload["args"]] end end end