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

Allow job with jid matching unique lock to pass unique check #105

Merged

Conversation

deltaroe
Copy link
Contributor

Related to #101

…al queue

It the job has the same jid as the existing unique lock allow it to pass.  Required for scheduled and retry jobs to run
@@ -71,10 +71,10 @@ def perform(_)

it 'enqueues previously scheduled job' do
QueueWorker.sidekiq_options unique: true
QueueWorker.perform_in(60 * 60, [1, 2])
jid = QueueWorker.perform_in(60 * 60, 1, 2)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test wasn't testing the same job moving from the scheduled to its normal queue. The signature for the perform_in method is def perform_in(interval, *args) https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/worker.rb#L45 Therefore the first job had a single argument of an array and the second job had two Fixnum arguments.

Removing the two brackets caused the test to fail identifying issue #101

@mhenrixon mhenrixon merged commit 6a66815 into mhenrixon:master Aug 19, 2015
@@ -63,7 +63,7 @@ def old_unique_for?

def new_unique_for?
connection do |conn|
return conn.set(payload_hash, item['jid'], nx: true, ex: expires_at)
return conn.set(payload_hash, item['jid'], nx: true, ex: expires_at) || conn.get(payload_hash) == item['jid']
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this works - because then a perform_async job cannot run if a perform_in job is scheduled (even if the scheduled job is hours in advance). I'd rather not have a lock on the scheduled queue at all then a behaviour like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pik It sounds like we have different use cases. In our environment we don't want a duplicate job to be able to run now if it's scheduled in the future even if it's several hours out.

Copy link
Contributor

Choose a reason for hiding this comment

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

@deltaroe This is how Sidekiq-unique-jobs always handled jobs before -- it's not a "fix" if you implement a strategy that works for you and breaks it for everyone else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants