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
43 changes: 30 additions & 13 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,28 @@ migration paths where possible.

## Table of Contents

* [Getting Started](#getting-started)
* [Basic Usage](#basic-usage)
* [Running a worker process](#running-a-worker-process)
* [Enqueuing Jobs](#enqueuing-jobs)
* [Operational Considerations](#operational-considerations)
* [Monitoring Jobs & Workers](#monitoring-jobs--workers)
* [Lifecycle Hooks](#lifecycle-hooks)
* [Priority-based Alerting Threshholds](#priority-based-alerting-threshholds)
* [Continuous Monitoring](#continuous-monitoring)
* [Configuration](#configuration)
* [Migrating from other ActiveJob backends](#migrating-from-other-activejob-backends)
* [Migrating from DelayedJob](#migrating-from-delayedjob)
* [How to Contribute](#how-to-contribute)
- [Delayed](#delayed)
- [Why `Delayed`?](#why-delayed)
- [Table of Contents](#table-of-contents)
- [Getting Started](#getting-started)
- [Installation](#installation)
- [Basic Usage](#basic-usage)
- [Running a worker process](#running-a-worker-process)
- [Enqueuing Jobs](#enqueuing-jobs)
- [Other ActiveJob Features](#other-activejob-features)
- [Operational Considerations](#operational-considerations)
- [Co-transactionality](#co-transactionality)
- [At-Least-Once Delivery](#at-least-once-delivery)
- [When Jobs Fail](#when-jobs-fail)
- [Monitoring Jobs \& Workers](#monitoring-jobs--workers)
- [Lifecycle Hooks](#lifecycle-hooks)
- [Priority-based Alerting Threshholds](#priority-based-alerting-threshholds)
- [Continuous Monitoring](#continuous-monitoring)
- [Configuration](#configuration)
- [Migrating from other ActiveJob backends](#migrating-from-other-activejob-backends)
- [Migrating from DelayedJob](#migrating-from-delayedjob)
- [How to Contribute](#how-to-contribute)
- [Suggested Workflow](#suggested-workflow)

## Getting Started

Expand Down Expand Up @@ -491,6 +500,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
42 changes: 38 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,14 @@ def <=>(other)
to_i <=> other
end

def -(other)
to_i - other.to_i
end

def +(other)
to_i + other.to_i
end
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about what it meant to always call to_i on other, and also what it means to always respond with an integer. For example:

Priority.new(1) + Priority.new(10)

I would expect that to return Priority.new(11), not 11.

And also these examples:

Priority.new(10) + 1

Should also probably return Priority.new(11).

But I'm not sure what this should return:

Priority.new(10) + 0.9

(Should it round to Priority.new(11), or raise an error?)

Maybe we don't need to handle that last case for now.

So I think that leaves us with this:

Suggested change
def -(other)
to_i - other.to_i
end
def +(other)
to_i + other.to_i
end
def -(other)
new(to_i - other.to_i)
end
def +(other)
new(to_i + other.to_i)
end

Though we could do what we do in <=> and cast other to_i only if it's a priority:

other = other.to_i if other.is_a?(self.class)

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah that's a good call! We should return a Priority instance, since that is the caller.

Hmm yeah the math complexities are interesting. I would say we probably kick the can on dealing with floats? Right now the initialize method (and the mid_point method) will ensure all instances have integer values.


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