Skip to content

Commit

Permalink
fix: Improve cooldown default values (ixti#195)
Browse files Browse the repository at this point in the history
See ixti#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 <[email protected]>
  • Loading branch information
bdegomme and ixti authored Nov 6, 2024
1 parent 39f3f78 commit f1a86f1
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 12 deletions.
6 changes: 6 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 15 additions & 4 deletions README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
4 changes: 2 additions & 2 deletions lib/sidekiq/throttled/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions sidekiq-throttled.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 2 additions & 2 deletions spec/lib/sidekiq/throttled/config_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion spec/lib/sidekiq/throttled/registry_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit f1a86f1

Please sign in to comment.