Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ gem 'resque_solo'
class UpdateCat
include Resque::Plugins::UniqueJob
@queue = :cats
@lock_after_execution_period = 20

def self.perform(cat_id)
# do something
Expand Down
17 changes: 0 additions & 17 deletions lib/resque_ext/resque.rb
Original file line number Diff line number Diff line change
@@ -1,22 +1,5 @@
module Resque
class << self
def enqueue_to(queue, klass, *args)
# Perform before_enqueue hooks. Don't perform enqueue if any hook returns false
before_hooks = Plugin.before_enqueue_hooks(klass).collect do |hook|
klass.send(hook, *args)
end
return nil if before_hooks.any? { |result| result == false }

result = Job.create(queue, klass, *args)
return nil if result == "EXISTED"

Plugin.after_enqueue_hooks(klass).each do |hook|
klass.send(hook, *args)
end

true
end

def enqueued?(klass, *args)
enqueued_in?(queue_from_class(klass), klass, *args)
end
Expand Down
8 changes: 8 additions & 0 deletions lib/resque_solo/unique_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ def ttl
def lock_after_execution_period
@lock_after_execution_period ||= 0
end

# We want this to run first in before_enqueue_hooks (which are alpha sorted), so name appropriately
def before_enqueue_001_solo_job(*args)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really excited to find resque_solo, the unreleased changes to make it not call after_enqueue_hooks if the job was not created, and this PR which makes the implementation hook based.

Why does the order of execution of before_enqueue hooks matter?

All before_enqueue_hooks will be invoked by Resque.enqueue_to, regardless of the result of any individual hook. So, it isn't clear to me why the order the hooks are invoked would matter.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you're totally right. I just removed the misleading comment and name.

queue_name = self.instance_variable_get(:@queue)
item = { class: self.to_s, args: args }
!ResqueSolo::Queue.queued?(queue_name, item)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we write this all as one line?

!ResqueSolo::Queue.queued?(@queue, class: to_s, args: args)

end

end
end
end
Expand Down
2 changes: 2 additions & 0 deletions resque_solo.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,7 @@ Gem::Specification.new do |spec|
spec.add_development_dependency "bundler", "~> 1.6"
spec.add_development_dependency "fakeredis", "~> 0.4"
spec.add_development_dependency "minitest", "~> 5.8"
spec.add_development_dependency "minitest-reporters", "~> 1.1"
spec.add_development_dependency "rake", "~> 11.2"
spec.add_development_dependency "m", "~> 1.5"
end
19 changes: 13 additions & 6 deletions test/resque_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ class ResqueTest < MiniTest::Spec
it "enqueues normal jobs" do
Resque.enqueue FakeJob, "x"
Resque.enqueue FakeJob, "x"
assert_equal 2, Resque.size(:normal)
queue_name = FakeJob.instance_variable_get(:@queue)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all these tests, I prefer to simply hard-code the queue name rather than accessing an internal instance variable. So don't change the tests to use instance_variable_get. There's no reason to change any of these tests except line 47.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, should be good now I think... I haven't yet figured out the workflow to rebase and squash commits yet so let me know if I need to figure out how to make that happen.

assert_equal 2, Resque.size(queue_name)
end

it "is not able to report if a non-unique job was enqueued" do
Expand All @@ -26,19 +27,25 @@ class ResqueTest < MiniTest::Spec
describe "#enqueue_to" do
describe "non-unique job" do
it "should return true if job was enqueued" do
assert Resque.enqueue_to(:normal, FakeJob)
assert Resque.enqueue_to(:normal, FakeJob)
queue_name = FakeJob.instance_variable_get(:@queue)
assert Resque.enqueue_to(queue_name, FakeJob)
assert Resque.enqueue_to(queue_name, FakeJob)
end
end

describe "unique job" do
before do
@queue_name = FakeUniqueJob.instance_variable_get(:@queue)
end

it "should return true if job was enqueued" do
assert Resque.enqueue_to(:normal, FakeUniqueJob)
assert Resque.enqueue_to(@queue_name, FakeUniqueJob)
end

it "should return nil if job already existed" do
Resque.enqueue_to(:normal, FakeUniqueJob)
assert_nil Resque.enqueue_to(:normal, FakeUniqueJob)
Resque.enqueue_to(@queue_name, FakeUniqueJob)
assert Resque.enqueued?(FakeUniqueJob)
assert_nil Resque.enqueue_to(@queue_name, FakeUniqueJob)
end
end
end
Expand Down
5 changes: 4 additions & 1 deletion test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,14 @@
end

require "minitest/autorun"
require "minitest/reporters"
require "resque_solo"
require "fake_jobs"
require "fakeredis"
require "fakeredis/minitest"
begin
require "pry-byebug"
rescue LoadError
# ignore
end

Minitest::Reporters.use! [Minitest::Reporters::DefaultReporter.new({ color: true })]