Permalink
Browse files

Move #run from Job to Worker. Refs #26

  • Loading branch information...
1 parent 86a638b commit 02d561ab5b5c2f244fb1cc996ab183fd94de1fdb @bkeepers bkeepers committed Dec 19, 2009
Showing with 89 additions and 159 deletions.
  1. +0 −34 lib/delayed/job.rb
  2. +22 −8 lib/delayed/worker.rb
  3. +1 −8 spec/delayed_method_spec.rb
  4. +2 −67 spec/job_spec.rb
  5. +64 −42 spec/worker_spec.rb
View
34 lib/delayed/job.rb
@@ -69,34 +69,6 @@ def reschedule(message, backtrace = [], time = nil)
end
end
-
- # Try to lock and run job. Returns true/false (work done/work failed) or nil if job can't be locked.
- def run_with_lock(max_run_time, worker_name)
- logger.info "* [JOB] acquiring lock on #{name}"
- if lock_exclusively!(max_run_time, worker_name)
- run(max_run_time)
- else
- # We did not get the lock, some other worker process must have
- logger.warn "* [JOB] failed to acquire exclusive lock for #{name}"
- nil # no work done
- end
- end
-
- # Try to run job. Returns true/false (work done/work failed)
- def run(max_run_time)
- runtime = Benchmark.realtime do
- Timeout.timeout(max_run_time.to_i) { invoke_job }
- destroy
- end
- # TODO: warn if runtime > max_run_time ?
- logger.info "* [JOB] #{name} completed after %.4f" % runtime
- return true # did work
- rescue Exception => e
- reschedule e.message, e.backtrace
- log_exception(e)
- return false # work failed
- end
-
# Add a job to the queue
def self.enqueue(*args, &block)
object = block_given? ? EvaledJob.new(&block) : args.shift
@@ -149,12 +121,6 @@ def unlock
self.locked_by = nil
end
- # This is a good hook if you need to report job processing errors in additional or different ways
- def log_exception(error)
- logger.error "* [JOB] #{name} failed with #{error.class.name}: #{error.message} - #{attempts} failed attempts"
- logger.error(error)
- end
-
# Moved into its own method so that new_relic can trace it.
def invoke_job
payload_object.perform
View
30 lib/delayed/worker.rb
@@ -70,15 +70,33 @@ def say(text, level = Logger::INFO)
end
protected
+
+ def run(job)
+ runtime = Benchmark.realtime do
+ Timeout.timeout(self.class.max_run_time.to_i) { job.invoke_job }
+ job.destroy
+ end
+ # TODO: warn if runtime > max_run_time ?
+ say "* [JOB] #{name} completed after %.4f" % runtime
+ return true # did work
+ rescue Exception => e
+ handle_failed_job(job, e)
+ return false # work failed
+ end
+
+ def handle_failed_job(job, error)
+ job.reschedule error.message, error.backtrace
+ say "* [JOB] #{name} failed with #{error.class.name}: #{error.message} - #{job.attempts} failed attempts", Logger::ERROR
+ end
# Run the next job we can get an exclusive lock on.
# If no jobs are left we return nil
- def reserve_and_run_one_job(max_run_time = self.class.max_run_time)
+ def reserve_and_run_one_job
# We get up to 5 jobs from the db. In case we cannot get exclusive access to a job we try the next.
# this leads to a more even distribution of jobs across the worker processes
- job = Delayed::Job.find_available(name, 5, max_run_time).detect do |job|
- if job.lock_exclusively!(max_run_time, name)
+ job = Delayed::Job.find_available(name, 5, self.class.max_run_time).detect do |job|
+ if job.lock_exclusively!(self.class.max_run_time, name)
say "* [Worker(#{name})] acquired lock on #{job.name}"
true
else
@@ -87,11 +105,7 @@ def reserve_and_run_one_job(max_run_time = self.class.max_run_time)
end
end
- if job.nil?
- nil # we didn't do any work, all 5 were not lockable
- else
- job.run(max_run_time)
- end
+ run(job) if job
end
# Do num jobs and return stats on success/failure.
View
9 spec/delayed_method_spec.rb
@@ -73,15 +73,8 @@ def read(story)
end
it "should ignore ActiveRecord::RecordNotFound errors because they are permanent" do
-
job = ErrorObject.new.send_later(:throw)
-
- Delayed::Job.count.should == 1
-
- job.run_with_lock(Delayed::Worker.max_run_time, 'worker')
-
- Delayed::Job.count.should == 0
-
+ lambda { job.invoke_job }.should_not raise_error
end
it "should store the object as string if its an active record" do
View
69 spec/job_spec.rb
@@ -43,16 +43,6 @@
Delayed::Job.first.run_at.should be_close(later, 1)
end
- it "should call perform on jobs when running run_with_lock" do
- SimpleJob.runs.should == 0
-
- job = Delayed::Job.enqueue SimpleJob.new
- job.run_with_lock(Delayed::Worker.max_run_time, 'worker')
-
- SimpleJob.runs.should == 1
- end
-
-
it "should work with eval jobs" do
$eval_job_ran = false
@@ -61,47 +51,16 @@
JOB
end
- job.run_with_lock(Delayed::Worker.max_run_time, 'worker')
+ job.invoke_job
$eval_job_ran.should == true
end
it "should work with jobs in modules" do
- M::ModuleJob.runs.should == 0
-
job = Delayed::Job.enqueue M::ModuleJob.new
- job.run_with_lock(Delayed::Worker.max_run_time, 'worker')
-
- M::ModuleJob.runs.should == 1
+ lambda { job.invoke_job }.should change { M::ModuleJob.runs }.from(0).to(1)
end
- it "should re-schedule by about 1 second at first and increment this more and more minutes when it fails to execute properly" do
- job = Delayed::Job.enqueue ErrorJob.new
- job.run_with_lock(Delayed::Worker.max_run_time, 'worker')
-
- job = Delayed::Job.find(:first)
-
- job.last_error.should =~ /did not work/
- job.last_error.should =~ /sample_jobs.rb:8:in `perform'/
- job.attempts.should == 1
-
- job.run_at.should > Delayed::Job.db_time_now - 10.minutes
- job.run_at.should < Delayed::Job.db_time_now + 10.minutes
- end
-
- it "should record last_error when destroy_failed_jobs = false, max_attempts = 1" do
- Delayed::Job.destroy_failed_jobs = false
- Delayed::Worker.max_attempts = 1
- job = Delayed::Job.enqueue ErrorJob.new
- job.run(1)
- job.reload
- job.last_error.should =~ /did not work/
- job.last_error.should =~ /job_spec.rb/
- job.attempts.should == 1
-
- job.failed_at.should_not == nil
- end
-
it "should raise an DeserializationError when the job class is totally unknown" do
job = Delayed::Job.new
@@ -183,13 +142,6 @@
end
end
- it "should fail after Worker.max_run_time" do
- @job = Delayed::Job.create :payload_object => LongRunningJob.new
- @job.run_with_lock(1.second, 'worker')
- @job.reload.last_error.should =~ /expired/
- @job.attempts.should == 1
- end
-
it "should never find failed jobs" do
@job = Delayed::Job.create :payload_object => SimpleJob.new, :attempts => 50, :failed_at => Delayed::Job.db_time_now
Delayed::Job.find_available('worker', 1).length.should == 0
@@ -294,23 +246,6 @@
end
- context "when pulling jobs off the queue for processing, it" do
- before(:each) do
- @job = Delayed::Job.create(
- :payload_object => SimpleJob.new,
- :locked_by => 'worker1',
- :locked_at => Delayed::Job.db_time_now - 5.minutes)
- end
-
- it "should leave the queue in a consistent state and not run the job if locking fails" do
- SimpleJob.runs.should == 0
- @job.stub!(:lock_exclusively!).with(any_args).once.and_return(false)
- @job.run_with_lock(Delayed::Worker.max_run_time, 'worker')
- SimpleJob.runs.should == 0
- end
-
- end
-
context "db_time_now" do
it "should return time in current time zone if set" do
Time.zone = 'Eastern Time (US & Canada)'
View
106 spec/worker_spec.rb
@@ -5,8 +5,9 @@ def job_create(opts = {})
Delayed::Job.create(opts.merge(:payload_object => SimpleJob.new))
end
- before do
- Delayed::Worker.class_eval('public :work_off')
+ before(:all) do
+ Delayed::Worker.send :public, :work_off
+ Delayed::Worker.send :public, :run
end
before(:each) do
@@ -16,7 +17,22 @@ def job_create(opts = {})
SimpleJob.runs = 0
end
-
+
+ describe "running a job" do
+ it "should fail after Worker.max_run_time" do
+ begin
+ old_max_run_time = Delayed::Worker.max_run_time
+ Delayed::Worker.max_run_time = 1.second
+ @job = Delayed::Job.create :payload_object => LongRunningJob.new
+ @worker.run(@job)
+ @job.reload.last_error.should =~ /expired/
+ @job.attempts.should == 1
+ ensure
+ Delayed::Worker.max_run_time = old_max_run_time
+ end
+ end
+ end
+
context "worker prioritization" do
before(:each) do
@worker = Delayed::Worker.new(:max_priority => 5, :min_priority => -5, :quiet => true)
@@ -44,55 +60,61 @@ def job_create(opts = {})
end
end
- context "while running alongside other workers that locked jobs, it" do
+ context "while running with locked and expired jobs" do
before(:each) do
@worker.name = 'worker1'
- job_create(:locked_by => 'worker1', :locked_at => (Delayed::Job.db_time_now - 1.minutes))
- job_create(:locked_by => 'worker2', :locked_at => (Delayed::Job.db_time_now - 1.minutes))
+ end
+
+ it "should not run jobs locked by another worker" do
+ job_create(:locked_by => 'other_worker', :locked_at => (Delayed::Job.db_time_now - 1.minutes))
+ lambda { @worker.work_off }.should_not change { SimpleJob.runs }
+ end
+
+ it "should run open jobs" do
job_create
- job_create(:locked_by => 'worker1', :locked_at => (Delayed::Job.db_time_now - 1.minutes))
+ lambda { @worker.work_off }.should change { SimpleJob.runs }.from(0).to(1)
end
-
- it "should ingore locked jobs from other workers" do
- @worker.name = 'worker3'
- SimpleJob.runs.should == 0
- @worker.work_off
- SimpleJob.runs.should == 1 # runs the one open job
+
+ it "should run expired jobs" do
+ expired_time = Delayed::Job.db_time_now - (1.minutes + Delayed::Worker.max_run_time)
+ job_create(:locked_by => 'other_worker', :locked_at => expired_time)
+ lambda { @worker.work_off }.should change { SimpleJob.runs }.from(0).to(1)
end
-
- it "should find our own jobs regardless of locks" do
- @worker.name = 'worker1'
- SimpleJob.runs.should == 0
- @worker.work_off
- SimpleJob.runs.should == 3 # runs open job plus worker1 jobs that were already locked
+
+ it "should run own jobs" do
+ job_create(:locked_by => @worker.name, :locked_at => (Delayed::Job.db_time_now - 1.minutes))
+ lambda { @worker.work_off }.should change { SimpleJob.runs }.from(0).to(1)
end
end
-
- context "while running with locked and expired jobs, it" do
- before(:each) do
- @worker.name = 'worker1'
- exp_time = Delayed::Job.db_time_now - (1.minutes + Delayed::Worker.max_run_time)
- job_create(:locked_by => 'worker1', :locked_at => exp_time)
- job_create(:locked_by => 'worker2', :locked_at => (Delayed::Job.db_time_now - 1.minutes))
- job_create
- job_create(:locked_by => 'worker1', :locked_at => (Delayed::Job.db_time_now - 1.minutes))
+
+ describe "failed jobs" do
+ before do
+ # reset defaults
+ Delayed::Job.destroy_failed_jobs = true
+ Delayed::Worker.max_attempts = 25
+
+ @job = Delayed::Job.enqueue ErrorJob.new
end
- it "should only find unlocked and expired jobs" do
- @worker.name = 'worker3'
- SimpleJob.runs.should == 0
- @worker.work_off
- SimpleJob.runs.should == 2 # runs the one open job and one expired job
+ it "should record last_error when destroy_failed_jobs = false, max_attempts = 1" do
+ Delayed::Job.destroy_failed_jobs = false
+ Delayed::Worker.max_attempts = 1
+ @worker.run(@job)
+ @job.reload
+ @job.last_error.should =~ /did not work/
+ @job.last_error.should =~ /worker_spec.rb/
+ @job.attempts.should == 1
+ @job.failed_at.should_not be_nil
end
-
- it "should ignore locks when finding our own jobs" do
- @worker.name = 'worker1'
- SimpleJob.runs.should == 0
- @worker.work_off
- SimpleJob.runs.should == 3 # runs open job plus worker1 jobs
- # This is useful in the case of a crash/restart on worker1, but make sure multiple workers on the same host have unique names!
+
+ it "should re-schedule jobs after failing" do
+ @worker.run(@job)
+ @job.reload
+ @job.last_error.should =~ /did not work/
+ @job.last_error.should =~ /sample_jobs.rb:8:in `perform'/
+ @job.attempts.should == 1
+ @job.run_at.should > Delayed::Job.db_time_now - 10.minutes
+ @job.run_at.should < Delayed::Job.db_time_now + 10.minutes
end
-
end
-
end

0 comments on commit 02d561a

Please sign in to comment.