From 7ebc7d17e13a69f149f81edf88169ec4d92417b2 Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Mon, 8 Jul 2024 12:27:03 +0200 Subject: [PATCH 1/3] Remove a bit of code around discrete distinction --- app/models/good_job/base_execution.rb | 19 ---------- app/models/good_job/job.rb | 2 - demo/db/seeds.rb | 4 +- lib/good_job.rb | 2 +- scripts/benchmark_discrete_jobs.rb | 38 ------------------- spec/app/models/good_job/job_spec.rb | 6 --- .../active_job_extensions/concurrency_spec.rb | 18 --------- spec/lib/good_job/adapter_spec.rb | 34 ----------------- spec/lib/good_job_spec.rb | 18 +-------- 9 files changed, 3 insertions(+), 138 deletions(-) delete mode 100644 scripts/benchmark_discrete_jobs.rb diff --git a/app/models/good_job/base_execution.rb b/app/models/good_job/base_execution.rb index 62cb6f48d..1490b9fa2 100644 --- a/app/models/good_job/base_execution.rb +++ b/app/models/good_job/base_execution.rb @@ -216,14 +216,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`. @@ -545,17 +537,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 diff --git a/app/models/good_job/job.rb b/app/models/good_job/job.rb index f34788699..05282c773 100644 --- a/app/models/good_job/job.rb +++ b/app/models/good_job/job.rb @@ -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 diff --git a/demo/db/seeds.rb b/demo/db/seeds.rb index b2526f626..dc75b1828 100644 --- a/demo/db/seeds.rb +++ b/demo/db/seeds.rb @@ -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 @@ -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 diff --git a/lib/good_job.rb b/lib/good_job.rb index b25ed027a..a1a71862d 100644 --- a/lib/good_job.rb +++ b/lib/good_job.rb @@ -269,7 +269,7 @@ 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? + true end # Deprecator for providing deprecation warnings. diff --git a/scripts/benchmark_discrete_jobs.rb b/scripts/benchmark_discrete_jobs.rb deleted file mode 100644 index ed70e7dac..000000000 --- a/scripts/benchmark_discrete_jobs.rb +++ /dev/null @@ -1,38 +0,0 @@ -ENV['GOOD_JOB_EXECUTION_MODE'] = 'external' - -require_relative '../demo/config/environment' -require_relative '../lib/good_job' -require 'benchmark/ips' - -performer = GoodJob::JobPerformer.new("*") -Benchmark.ips do |x| - GoodJob::Execution.delete_all - ActiveJob::Base.queue_adapter.enqueue_all 10_000.times.map { ExampleJob.new } - GoodJob::Execution.update_all(is_discrete: true) - x.report("discrete jobs and no errors") do - performer.next - end - - GoodJob::Execution.delete_all - ActiveJob::Base.queue_adapter.enqueue_all 10_000.times.map { ExampleJob.new } - GoodJob::Execution.update_all(is_discrete: false) - x.report("undiscrete jobs and no errors") do - performer.next - end - - GoodJob::Execution.delete_all - ActiveJob::Base.queue_adapter.enqueue_all 10_000.times.map { ExampleJob.new(ExampleJob::ERROR_FIVE_TIMES_TYPE) } - GoodJob::Execution.update_all(is_discrete: true) - x.report("discrete jobs and 5 errors") do - performer.next - end - - GoodJob::Execution.delete_all - ActiveJob::Base.queue_adapter.enqueue_all 10_000.times.map { ExampleJob.new(ExampleJob::ERROR_FIVE_TIMES_TYPE) } - GoodJob::Execution.update_all(is_discrete: false) - x.report("undiscrete jobs and 5 errors") do - performer.next - end - - x.compare! -end diff --git a/spec/app/models/good_job/job_spec.rb b/spec/app/models/good_job/job_spec.rb index e2e675b96..52badc103 100644 --- a/spec/app/models/good_job/job_spec.rb +++ b/spec/app/models/good_job/job_spec.rb @@ -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' @@ -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 @@ -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 diff --git a/spec/lib/good_job/active_job_extensions/concurrency_spec.rb b/spec/lib/good_job/active_job_extensions/concurrency_spec.rb index d58f7b8e6..e42de273d 100644 --- a/spec/lib/good_job/active_job_extensions/concurrency_spec.rb +++ b/spec/lib/good_job/active_job_extensions/concurrency_spec.rb @@ -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 diff --git a/spec/lib/good_job/adapter_spec.rb b/spec/lib/good_job/adapter_spec.rb index 19cc515d4..6c238240b 100644 --- a/spec/lib/good_job/adapter_spec.rb +++ b/spec/lib/good_job/adapter_spec.rb @@ -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) @@ -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 @@ -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) @@ -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 @@ -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]) @@ -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 diff --git a/spec/lib/good_job_spec.rb b/spec/lib/good_job_spec.rb index 098e4f230..1889e5630 100644 --- a/spec/lib/good_job_spec.rb +++ b/spec/lib/good_job_spec.rb @@ -164,24 +164,8 @@ def perform(succeed: true) end describe '#v4_ready?' do - it "is true when all jobs are discrete" do + it "is true" do 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 - end end end From 76ddc35f65ca8937cddb5038aabdeb19753ba2fa Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Mon, 8 Jul 2024 12:33:41 +0200 Subject: [PATCH 2/3] Ignore the `is_discrete` column For a safe migration, this column must be ignored first --- app/models/good_job/base_execution.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/good_job/base_execution.rb b/app/models/good_job/base_execution.rb index 1490b9fa2..36008daf6 100644 --- a/app/models/good_job/base_execution.rb +++ b/app/models/good_job/base_execution.rb @@ -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 From cb505ef7038b0898edcdc1111cbac291b386f8b2 Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Mon, 8 Jul 2024 18:14:27 +0200 Subject: [PATCH 3/3] Deprecate `GoodJob.v4_ready?` If you're reading this deprecation you're already all good (: --- lib/good_job.rb | 4 ++++ spec/lib/good_job_spec.rb | 2 ++ 2 files changed, 6 insertions(+) diff --git a/lib/good_job.rb b/lib/good_job.rb index a1a71862d..23e11e0c5 100644 --- a/lib/good_job.rb +++ b/lib/good_job.rb @@ -269,6 +269,10 @@ def self.perform_inline(queue_string = "*", limit: nil) # Tests whether GoodJob can be safely upgraded to v4 # @return [Boolean] def self.v4_ready? + 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 diff --git a/spec/lib/good_job_spec.rb b/spec/lib/good_job_spec.rb index 1889e5630..0005de2ee 100644 --- a/spec/lib/good_job_spec.rb +++ b/spec/lib/good_job_spec.rb @@ -165,7 +165,9 @@ def perform(succeed: true) describe '#v4_ready?' do it "is true" do + allow(described_class.deprecator).to receive(:warn) expect(described_class.v4_ready?).to eq true + expect(described_class.deprecator).to have_received(:warn) end end end