Skip to content

Commit

Permalink
do not register non-scheduled (e.g. unique-evicted) jobs in batches
Browse files Browse the repository at this point in the history
  • Loading branch information
alachaum committed Jun 18, 2024
1 parent 1e0f6e5 commit 3362dbb
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 8 deletions.
4 changes: 3 additions & 1 deletion lib/cloudtasker/batch/job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,9 @@ def schedule_pending_jobs

while (j = pending_jobs.shift)
# Schedule the job
j.schedule
# Skip batch registration if the job was not actually scheduled
# E.g. the job was evicted due to uniqueness requirements
next unless j.schedule

# Initialize the batch state unless the job has already started (and taken
# hold of its own status)
Expand Down
6 changes: 4 additions & 2 deletions lib/cloudtasker/unique_job/conflict_strategy/base_strategy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,18 @@ def initialize(job)
# Handling logic to perform when a conflict occurs while
# scheduling a job.
#
# We return nil to flag the job as not scheduled
#
def on_schedule
true
nil
end

#
# Handling logic to perform when a conflict occurs while
# executing a job.
#
def on_execute
true
nil
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/cloudtasker/worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ def schedule_time(interval: nil, time_at: nil)
# @param [Integer] interval The delay in seconds.
# @param [Time, Integer] interval The time at which the job should run
#
# @return [Cloudtasker::CloudTask] The Google Task response
# @return [Cloudtasker::CloudTask, nil] The Google Task response or nil if the job was not scheduled
#
def schedule(**args)
# Evaluate when to schedule the job
Expand Down
21 changes: 19 additions & 2 deletions spec/cloudtasker/batch/job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@
context 'with pending jobs' do
before do
batch.pending_jobs.push(child_worker)
expect(child_worker).to receive(:schedule)
expect(child_worker).to receive(:schedule).and_return(instance_double(Cloudtasker::CloudTask))
schedule_pending_jobs
end
after do
Expand All @@ -270,7 +270,7 @@
context 'with job having completed even before being flagged as scheduled' do
before do
batch.pending_jobs.push(child_worker)
expect(child_worker).to receive(:schedule)
expect(child_worker).to receive(:schedule).and_return(instance_double(Cloudtasker::CloudTask))
redis.hset(batch.batch_state_gid, child_worker.job_id, 'completed')
schedule_pending_jobs
end
Expand All @@ -283,6 +283,21 @@
it { is_expected.to eq([child_worker]) }
end

context 'with non-scheduled jobs (e.g. unique-evicted jobs)' do
before do
batch.pending_jobs.push(child_worker)
expect(child_worker).to receive(:schedule).and_return(nil)
schedule_pending_jobs
end
after do
expect(batch_state).to be_empty
expect(batch.pending_jobs).to be_empty
expect(batch.enqueued_jobs).to be_empty
end

it { is_expected.to be_empty }
end

context 'with no pending_jobs' do
after do
expect(batch_state).to be_empty
Expand Down Expand Up @@ -572,6 +587,7 @@
before do
# Do not enqueue jobs
allow_any_instance_of(Cloudtasker::Worker).to receive(:schedule)
.and_return(instance_double(Cloudtasker::CloudTask))

# Create un-related batch
side_batch.pending_jobs.push(worker.new_instance)
Expand Down Expand Up @@ -602,6 +618,7 @@
before do
# Stub job enqueuing
allow_any_instance_of(Cloudtasker::Worker).to receive(:schedule)
.and_return(instance_double(Cloudtasker::CloudTask))

# Add child jobs
child_batch.pending_jobs.push(worker.new_instance)
Expand Down
4 changes: 2 additions & 2 deletions spec/cloudtasker/unique_job/conflict_strategy/reject_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@
describe '#on_schedule' do
subject { strategy.on_schedule }

it { is_expected.to be_truthy }
it { is_expected.to be_falsey }
it { expect { |b| strategy.on_schedule(&b) }.not_to yield_control }
end

describe '#on_execute' do
subject { strategy.on_execute }

it { is_expected.to be_truthy }
it { is_expected.to be_falsey }
it { expect { |b| strategy.on_execute(&b) }.not_to yield_control }
end
end

0 comments on commit 3362dbb

Please sign in to comment.