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

Remove smaller_number_is_higher_priority option from v4 #1453

Merged
merged 1 commit into from
Aug 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,6 @@ Available configuration options are:
- `inline_execution_respects_schedule` (boolean) Opt-in to future behavior of inline execution respecting scheduled jobs. Defaults to `false`.
- `logger` ([Rails Logger](https://api.rubyonrails.org/classes/ActiveSupport/Logger.html)) lets you set a custom logger for GoodJob. It should be an instance of a Rails `Logger` (Default: `Rails.logger`).
- `preserve_job_records` (boolean) keeps job records in your database even after jobs are completed. (Default: `true`)
- `smaller_number_is_higher_priority` (boolean) allows you to specifiy that jobs should be run in ascending order of priority (smallest priority numbers first). This will be enabled by default in the next major version of GoodJob (v4.0), but jobs with the highest priority number are run first by default in all earlier versions of GoodJob.
- `retry_on_unhandled_error` (boolean) causes jobs to be re-queued and retried if they raise an instance of `StandardError`. Be advised this may lead to jobs being repeated infinitely ([see below for more on retries](#retries)). Instances of `Exception`, like SIGINT, will *always* be retried, regardless of this attribute’s value. (Default: `false`)
- `on_thread_error` (proc, lambda, or callable) will be called when there is an Exception. It can be useful for logging errors to bug tracking services, like Sentry or Airbrake. Example:

Expand Down Expand Up @@ -499,7 +498,9 @@ As a second example, you may wish to show a link to a log aggregator next to eac

### Job priority

Higher priority numbers run first in all versions of GoodJob v3.x and below. GoodJob v4.x will change job `priority` to give smaller numbers higher priority (default: `0`), in accordance with Active Job's definition of priority (see #524). To opt-in to this behavior now, set `config.good_job.smaller_number_is_higher_priority = true` in your GoodJob initializer or `application.rb`.
Smaller `priority` values have higher priority and run first (default: `0`), in accordance with [Active Job's definition of priority](https://github.com/rails/rails/blob/e17faead4f2aff28da079d50f02ea5b015322d5b/activejob/lib/active_job/core.rb#L22).

Prior to GoodJob v4, this was reversed: higher priority numbers ran first in all versions of GoodJob v3.x and below. When migrating from v3 to v4, new behavior can be opted into by setting `config.good_job.smaller_number_is_higher_priority = true` in your GoodJob initializer or `application.rb`.

### Labelled jobs

Expand Down
8 changes: 1 addition & 7 deletions app/models/good_job/job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,7 @@ class Job < BaseRecord
# @!method priority_ordered
# @!scope class
# @return [ActiveRecord::Relation]
scope :priority_ordered, (lambda do
if GoodJob.configuration.smaller_number_is_higher_priority
order('priority ASC NULLS LAST')
else
order('priority DESC NULLS LAST')
end
end)
scope :priority_ordered, -> { order('priority ASC NULLS LAST') }

# Order jobs by created_at, for first-in first-out
# @!method creation_ordered
Expand Down
8 changes: 0 additions & 8 deletions lib/good_job/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ class Configuration
DEFAULT_DASHBOARD_LIVE_POLL_ENABLED = true
# Default enqueue_after_transaction_commit
DEFAULT_ENQUEUE_AFTER_TRANSACTION_COMMIT = false
# Default smaller_number_is_higher_priority
DEFAULT_SMALLER_NUMBER_IS_HIGHER_PRIORITY = true

def self.validate_execution_mode(execution_mode)
raise ArgumentError, "GoodJob execution mode must be one of #{EXECUTION_MODES.join(', ')}. It was '#{execution_mode}' which is not valid." unless execution_mode.in?(EXECUTION_MODES)
Expand Down Expand Up @@ -347,12 +345,6 @@ def enable_listen_notify
DEFAULT_ENABLE_LISTEN_NOTIFY
end

def smaller_number_is_higher_priority
return rails_config[:smaller_number_is_higher_priority] unless rails_config[:smaller_number_is_higher_priority].nil?

DEFAULT_SMALLER_NUMBER_IS_HIGHER_PRIORITY
end

def dashboard_default_locale
rails_config[:dashboard_default_locale] || DEFAULT_DASHBOARD_DEFAULT_LOCALE
end
Expand Down
12 changes: 1 addition & 11 deletions spec/app/models/good_job/job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -566,20 +566,10 @@ def job_params
let!(:small_priority_job) { described_class.create!(priority: -50) }
let!(:large_priority_job) { described_class.create!(priority: 50) }

it 'smaller_number_is_higher_priority=true orders with smaller number being HIGHER priority' do
it 'orders with smaller number being HIGHER priority' do
allow(Rails.application.config).to receive(:good_job).and_return({ smaller_number_is_higher_priority: true })
expect(described_class.priority_ordered.pluck(:priority)).to eq([-50, 50])
end

it 'smaller_number_is_higher_priority=false orders with smaller number being LOWER priority' do
allow(Rails.application.config).to receive(:good_job).and_return({ smaller_number_is_higher_priority: false })
expect(described_class.priority_ordered.pluck(:priority)).to eq([50, -50])
end

it 'smaller_number_is_higher_priority=nil orders with smaller number being HIGHER priority' do
allow(Rails.application.config).to receive(:good_job).and_return({})
expect(described_class.priority_ordered.pluck(:priority)).to eq([-50, 50])
end
end

describe '.next_scheduled_at' do
Expand Down
8 changes: 0 additions & 8 deletions spec/lib/good_job/configuration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -298,14 +298,6 @@
end
end

describe '#smaller_number_is_higher_priority' do
it 'delegates to rails configuration' do
allow(Rails.application.config).to receive(:good_job).and_return({ smaller_number_is_higher_priority: true })
configuration = described_class.new({})
expect(configuration.smaller_number_is_higher_priority).to be true
end
end

describe '#dashboard_default_locale' do
it 'delegates to rails configuration' do
allow(Rails.application.config).to receive(:good_job).and_return({ dashboard_default_locale: :de })
Expand Down