Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuring default priority value at midpoint of range #39

Merged
merged 10 commits into from
Apr 29, 2024
2 changes: 1 addition & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
PATH
remote: .
specs:
delayed (0.5.3)
delayed (0.5.4)
activerecord (>= 5.2)
concurrent-ruby

Expand Down
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,14 @@ Delayed::Worker.min_priority = nil
Delayed::Worker.max_priority = nil
```

Job priorities can specified by using the name of the desired range (i.e. :user_visible).
By default, the value for a named priority will be the first value in that range.
To set each priority's default value to the middle of its range (i.e. 15 for :user_visible), Delayed::Priority can be configured with:

```ruby
Delayed::Priority.assign_at_midpoint = true
```

Comment on lines +494 to +501
Copy link
Member

@smudge smudge Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this makes sense. At some point we should consider making this the default, but opt-in is good for now.

Logging verbosity is also configurable. The gem will attempt to default to `Rails.logger` with an
"info" log level.

Expand Down
2 changes: 1 addition & 1 deletion delayed.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Gem::Specification.new do |spec|
spec.require_paths = ['lib']
spec.summary = 'a multi-threaded, SQL-driven ActiveJob backend used at Betterment to process millions of background jobs per day'

spec.version = '0.5.3'
spec.version = '0.5.4'
spec.metadata = {
'changelog_uri' => 'https://github.com/betterment/delayed/blob/main/CHANGELOG.md',
'bug_tracker_uri' => 'https://github.com/betterment/delayed/issues',
Expand Down
48 changes: 44 additions & 4 deletions lib/delayed/priority.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ class Priority < Numeric
}.freeze

class << self
attr_writer :assign_at_midpoint

def names
@names || default_names
end
Expand All @@ -70,6 +72,7 @@ def names=(names)

@ranges = nil
@alerts = nil
@names_to_priority = nil
@names = names&.sort_by(&:last)&.to_h&.transform_values { |v| new(v) }
end

Expand All @@ -82,12 +85,25 @@ def alerts=(alerts)
@alerts = alerts&.sort_by { |k, _| names.keys.index(k) }&.to_h
end

def assign_at_midpoint?
@assign_at_midpoint || false
end

def ranges
@ranges ||= names.zip(names.except(names.keys.first)).each_with_object({}) do |((name, lower), (_, upper)), obj|
obj[name] = (lower...(upper || Float::INFINITY))
end
end

def names_to_priority
tkoar marked this conversation as resolved.
Show resolved Hide resolved
@names_to_priority ||=
if assign_at_midpoint?
tkoar marked this conversation as resolved.
Show resolved Hide resolved
names_to_midpoint_priority
else
names
end
end

private

def default_names
Expand All @@ -98,13 +114,23 @@ def default_alerts
@names ? {} : DEFAULT_ALERTS
end

def names_to_midpoint_priority
names.each_cons(2).to_h { |(name, priority_value), (_, next_priority_value)|
[name, new(midpoint(priority_value, next_priority_value))]
}.merge(names.keys.last => new(names.values.last + 5))
end

def midpoint(low, high)
low + ((high - low).to_d / 2).ceil
end

def respond_to_missing?(method_name, include_private = false)
names.key?(method_name) || super
names_to_priority.key?(method_name) || super
end

def method_missing(method_name, *args)
if names.key?(method_name) && args.none?
names[method_name]
if names_to_priority.key?(method_name) && args.none?
names_to_priority[method_name]
else
super
end
Expand All @@ -118,7 +144,7 @@ def method_missing(method_name, *args)

def initialize(value)
super()
value = self.class.names[value] if value.is_a?(Symbol)
value = self.class.names_to_priority[value] if value.is_a?(Symbol)
tkoar marked this conversation as resolved.
Show resolved Hide resolved
@value = value.to_i
end

Expand Down Expand Up @@ -147,6 +173,20 @@ def <=>(other)
to_i <=> other
end

def -(other)
other = other.to_i if other.is_a?(self.class)
self.class.new(to_i - other)
end

def +(other)
other = other.to_i if other.is_a?(self.class)
self.class.new(to_i + other)
end

def to_d
to_i.to_d
end

private

def respond_to_missing?(method_name, include_private = false)
Expand Down
86 changes: 85 additions & 1 deletion spec/delayed/priority_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
RSpec.describe Delayed::Priority do
let(:custom_names) { nil }
let(:custom_alerts) { nil }
let(:assign_at_midpoint) { nil }

around do |example|
described_class.assign_at_midpoint = assign_at_midpoint
described_class.names = custom_names
described_class.alerts = custom_alerts
example.run
Expand All @@ -13,7 +15,7 @@
described_class.names = nil
end

describe '.names, .ranges, .alerts, method_missing' do
describe '.names, .ranges, .alerts, .names_to_priority, method_missing' do
it 'defaults to interactive, user_visible, eventual, reporting' do
expect(described_class.names).to eq(
interactive: 0,
Expand All @@ -33,6 +35,12 @@
eventual: { age: 1.5.hours, run_time: 5.minutes, attempts: 8 },
reporting: { age: 4.hours, run_time: 10.minutes, attempts: 8 },
)
expect(described_class.names_to_priority).to eq(
interactive: 0,
user_visible: 10,
eventual: 20,
reporting: 30,
)
expect(described_class).to respond_to(:interactive)
expect(described_class).to respond_to(:user_visible)
expect(described_class).to respond_to(:eventual)
Expand All @@ -43,6 +51,23 @@
expect(described_class.reporting).to eq 30
end

context 'when assign_at_midpoint is set to true' do
let(:assign_at_midpoint) { true }

it 'returns the midpoint value' do
expect(described_class.names_to_priority).to eq(
interactive: 5,
user_visible: 15,
eventual: 25,
reporting: 35,
)
expect(described_class.interactive).to eq 5
expect(described_class.user_visible).to eq 15
expect(described_class.eventual).to eq 25
expect(described_class.reporting).to eq 35
end
tkoar marked this conversation as resolved.
Show resolved Hide resolved
end

context 'when customized to high, medium, low' do
let(:custom_names) { { high: 0, medium: 100, low: 500 } }

Expand All @@ -57,6 +82,11 @@
medium: (100...500),
low: (500...Float::INFINITY),
)
expect(described_class.names_to_priority).to eq(
high: 0,
medium: 100,
low: 500,
)
expect(described_class.alerts).to eq({})
expect(described_class).not_to respond_to(:interactive)
expect(described_class).not_to respond_to(:user_visible)
Expand All @@ -81,6 +111,21 @@
)
end
end

context 'when assign_at_midpoint is set to true' do
let(:assign_at_midpoint) { true }

it 'returns the midpoint value' do
expect(described_class.names_to_priority).to eq(
high: 50,
medium: 300,
low: 505,
)
expect(described_class.high).to eq 50
expect(described_class.medium).to eq 300
expect(described_class.low).to eq 505
end
end
end
end

Expand Down Expand Up @@ -110,6 +155,36 @@
expect(described_class.new(-123).interactive?).to eq false
end

context 'when assign_at_midpoint is set to true' do
let(:assign_at_midpoint) { true }

it 'provides the name of the priority range' do
expect(described_class.new(0).name).to eq :interactive
expect(described_class.new(3).name).to eq :interactive
expect(described_class.new(10).name).to eq :user_visible
expect(described_class.new(29).name).to eq :eventual
expect(described_class.new(999).name).to eq :reporting
expect(described_class.new(-123).name).to eq nil
end

it 'supports initialization by symbol value' do
expect(described_class.new(:interactive)).to eq(5)
expect(described_class.new(:user_visible)).to eq(15)
expect(described_class.new(:eventual)).to eq(25)
expect(described_class.new(:reporting)).to eq(35)
end

it "supports predicate ('?') methods" do
expect(described_class.new(0).interactive?).to eq true
expect(described_class.new(3)).to be_interactive
expect(described_class.new(3).user_visible?).to eq false
expect(described_class.new(10)).to be_user_visible
expect(described_class.new(29)).to be_eventual
expect(described_class.new(999)).to be_reporting
expect(described_class.new(-123).interactive?).to eq false
end
end

it 'supports alert threshold methods' do
described_class.alerts = {
interactive: { age: 77.seconds },
Expand Down Expand Up @@ -151,4 +226,13 @@
].sort,
).to eq [-13, 3, 5, 40]
end

it 'supports addition and subtraction' do
expect(described_class.new(0) + 10).to eq(10)
expect(10 + described_class.new(5)).to eq(15)
expect(described_class.new(0) + described_class.new(33)).to eq(33)
expect(described_class.new(10) - 5).to eq(5)
expect(15 - described_class.new(10)).to eq(5)
expect(described_class.new(5) - described_class.new(15)).to eq(-10)
end
end
Loading