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

Discrete cleanup #1401

Merged
merged 3 commits into from
Jul 8, 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
21 changes: 2 additions & 19 deletions app/models/good_job/base_execution.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class BaseExecution < BaseRecord
self.advisory_lockable_column = 'active_job_id'
self.implicit_order_column = 'created_at'

self.ignored_columns += ["is_discrete"]

define_model_callbacks :perform
define_model_callbacks :perform_unlocked, only: :after

Expand Down Expand Up @@ -216,14 +218,6 @@ def params_execution_count
def coalesce_scheduled_at_created_at
arel_table.coalesce(arel_table['scheduled_at'], arel_table['created_at'])
end

def discrete_support?
true
end
end

def discrete?
is_discrete?
end

# Build an ActiveJob instance and deserialize the arguments, using `#active_job_data`.
Expand Down Expand Up @@ -545,17 +539,6 @@ def executable?
false
end

def make_discrete
self.is_discrete = true
self.id = active_job_id
self.job_class = serialized_params['job_class']
self.executions_count ||= 0

current_time = Time.current
self.created_at ||= current_time
self.scheduled_at ||= current_time
end

# Return formatted serialized_params for display in the dashboard
# @return [Hash]
def display_serialized_params
Expand Down
2 changes: 0 additions & 2 deletions app/models/good_job/job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@ class Job < BaseExecution
# Errored but will not be retried
scope :discarded, -> { finished.where.not(error: nil) }

scope :unfinished_undiscrete, -> { where(finished_at: nil, retried_good_job_id: nil, is_discrete: [nil, false]) }

# TODO: it would be nice to enforce these values at the model
# validates :active_job_id, presence: true
# validates :scheduled_at, presence: true
Expand Down
4 changes: 1 addition & 3 deletions demo/db/seeds.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
time_increments = (1.minute..10.minutes).to_a
job_classes = ['ExampleJob', 'OtherJob']
queue_names = ["default", "mice", "elephants"]
discrete_is_supported = GoodJob::BaseExecution.discrete_support?

jobs_data = []
loop do
Expand Down Expand Up @@ -36,8 +35,7 @@
scheduled_at: nil,
performed_at: nil,
finished_at: nil,
error: nil,
is_discrete: discrete_is_supported
error: nil
}

start_date += time_increments.sample
Expand Down
6 changes: 5 additions & 1 deletion lib/good_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,11 @@ def self.perform_inline(queue_string = "*", limit: nil)
# Tests whether GoodJob can be safely upgraded to v4
# @return [Boolean]
def self.v4_ready?
GoodJob::Job.discrete_support? && GoodJob::Job.where(finished_at: nil).where(is_discrete: [nil, false]).none?
GoodJob.deprecator.warn(<<~MSG)
Calling `GoodJob.v4_ready?` is deprecated and will be removed in GoodJob v5.
If you are reading this deprecation you are already on v4.
MSG
true
end

# Deprecator for providing deprecation warnings.
Expand Down
38 changes: 0 additions & 38 deletions scripts/benchmark_discrete_jobs.rb

This file was deleted.

6 changes: 0 additions & 6 deletions spec/app/models/good_job/job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -359,8 +359,6 @@ def perform(*)
end

before do
allow(described_class).to receive(:discrete_support?).and_return(false)

stub_const "RUN_JOBS", Concurrent::Array.new
stub_const 'TestJob', (Class.new(ActiveJob::Base) do
self.queue_name = 'test'
Expand Down Expand Up @@ -951,8 +949,6 @@ def job_params
context 'when Discrete' do
before do
ActiveJob::Base.queue_adapter = GoodJob::Adapter.new(execution_mode: :inline)
allow(described_class).to receive(:discrete_support?).and_return(true)
good_job.update!(is_discrete: true)
end

it 'updates the Execution record and creates a DiscreteExecution record' do
Expand Down Expand Up @@ -983,10 +979,8 @@ def job_params
let!(:good_job) { described_class.enqueue(active_job) }

before do
allow(described_class).to receive(:discrete_support?).and_return(true)
allow(GoodJob).to receive(:preserve_job_records).and_return(true)
TestJob.retry_on(StandardError, wait: 1.hour, attempts: 2) { nil }
good_job.update!(is_discrete: true)
end

it 'updates the existing Execution/Job record instead of creating a new one' do
Expand Down
18 changes: 0 additions & 18 deletions spec/lib/good_job/active_job_extensions/concurrency_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -248,24 +248,6 @@ def perform(*)
expect(GoodJob::Job.first.concurrency_key).to be_present
expect(GoodJob::Job.first).not_to be_finished
end

context 'when not discrete' do
it 'preserves the key value across retries' do
TestJob.set(wait_until: 5.minutes.ago).perform_later(name: "Alice")
GoodJob::Job.first.update!(is_discrete: false)

original_concurrency_key = GoodJob::Job.last.concurrency_key
expect(original_concurrency_key).to be_present

begin
GoodJob.perform_inline
rescue StandardError
nil
end

expect(GoodJob::Job.last.concurrency_key).to eq original_concurrency_key
end
end
end
end

Expand Down
34 changes: 0 additions & 34 deletions spec/lib/good_job/adapter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@

describe '#enqueue' do
it 'sets default values' do
allow(GoodJob::Job).to receive(:discrete_support?).and_return(true)

active_job = ExampleJob.new
adapter.enqueue(active_job)

Expand All @@ -37,16 +35,6 @@
priority: 0,
scheduled_at: be_within(1.second).of(Time.current)
)

allow(GoodJob::Job).to receive(:discrete_support?).and_return(false)
active_job = ExampleJob.new
adapter.enqueue(active_job)

expect(GoodJob::Job.last).to have_attributes(
queue_name: 'default',
priority: 0
# scheduled_at: be_within(1.second).of(Time.current) # TODO: this should be expected
)
end

it 'calls GoodJob::Job.enqueue with parameters' do
Expand Down Expand Up @@ -129,7 +117,6 @@ def perform(succeed: true)
end

it 'sets default values' do
allow(GoodJob::Job).to receive(:discrete_support?).and_return(true)
active_job = ExampleJob.new
adapter.enqueue_at(active_job, nil)

Expand All @@ -138,16 +125,6 @@ def perform(succeed: true)
priority: 0,
scheduled_at: be_within(1.second).of(Time.current)
)

allow(GoodJob::Job).to receive(:discrete_support?).and_return(false)
active_job = ExampleJob.new
adapter.enqueue_at(active_job, nil)

expect(GoodJob::Job.last).to have_attributes(
queue_name: 'default',
priority: 0
# scheduled_at: be_within(1.second).of(Time.current) # TODO: this should be expected
)
end
end

Expand Down Expand Up @@ -181,7 +158,6 @@ def perform(succeed: true)
end

it 'sets default values' do
allow(GoodJob::Job).to receive(:discrete_support?).and_return(true)
active_job = ExampleJob.new
adapter.enqueue_all([active_job])

Expand All @@ -190,16 +166,6 @@ def perform(succeed: true)
priority: 0,
scheduled_at: be_within(1.second).of(Time.current)
)

allow(GoodJob::Job).to receive(:discrete_support?).and_return(false)
active_job = ExampleJob.new
adapter.enqueue_at(active_job, nil)

expect(GoodJob::Job.last).to have_attributes(
queue_name: 'default',
priority: 0
# scheduled_at: be_within(1.second).of(Time.current) # TODO: this should be expected
)
end

context 'when a job fails to enqueue' do
Expand Down
20 changes: 3 additions & 17 deletions spec/lib/good_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -164,24 +164,10 @@ def perform(succeed: true)
end

describe '#v4_ready?' do
it "is true when all jobs are discrete" do
it "is true" do
allow(described_class.deprecator).to receive(:warn)
expect(described_class.v4_ready?).to eq true
end

it "is false when a job is not discrete" do
GoodJob::BaseRecord.uncached do
job = GoodJob::Job.create!(finished_at: nil, is_discrete: false)
expect(described_class.v4_ready?).to eq false

job.update! finished_at: nil, is_discrete: nil
expect(described_class.v4_ready?).to eq false

job.update! finished_at: nil, is_discrete: true
expect(described_class.v4_ready?).to eq true

job.update! finished_at: Time.current, is_discrete: nil
expect(described_class.v4_ready?).to eq true
end
expect(described_class.deprecator).to have_received(:warn)
end
end
end