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

added lock option for enqueue and retry methods (#7) #22

Closed
wants to merge 1 commit into from
Closed

added lock option for enqueue and retry methods (#7) #22

wants to merge 1 commit into from

Conversation

avkhozov
Copy link
Contributor

No description provided.

@kraih
Copy link
Member

kraih commented Jan 17, 2016

The last time this feature was discussed it did not have a lot of support.

@avkhozov
Copy link
Contributor Author

Yes, I see. But I think minion not yet such popular as mojolicious for example, and it now has a small feedback.

@kraih
Copy link
Member

kraih commented Jan 17, 2016

Well, personally i have no need for this feature, so maybe i should start with a 👎 vote. Especially since i've not seen anything similar in other job queues yet, so this is not a proven approach.

@jberger
Copy link
Member

jberger commented Jan 17, 2016

What is the performance impact when this feature is not used?

@avkhozov
Copy link
Contributor Author

I have already described example of usage in #7 (comment)

For example, you have two servers with the same configuration (mojo app + minion worker) and you need run periodically jobs (enqueued via cron). So you must solve the problem with duplication of same tasks. Without lock option in minion you need to manage this manually by creating extra tables/collections in your app and write logic for deduplication and cleaning old lock keys, etc. In minion backends this problems solved easier and minion more suited for this.

Of course, this is only my opinion.

$options->{delay} // 0, $options->{priority} // 0,
$options->{queue} // 'default', $task
)->hash->{id};
my $result = eval {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't an ON CONFLICT DO NOTHING work much better than eval?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, here it would be appropriate. But it requires a postgres 9.5.

Copy link
Member

Choose a reason for hiding this comment

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

Minion will soon require PostgreSQL 9.5 anyway for SKIP LOCKED.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll fix this when config for travis will be improved.

@kraih
Copy link
Member

kraih commented Jan 17, 2016

Regarding what features to accept, as long as we don't have more users willing to vote, i think features will be limited to what i need and/or what most other job queues support.

@avkhozov
Copy link
Contributor Author

Also there are recurring/periodic jobs (https://github.com/mperham/sidekiq/wiki/Ent-Periodic-Jobs) that cover my needs with lock option. I think it's more general case, which can enjoy more popularity.

@kraih
Copy link
Member

kraih commented Jan 18, 2016

Yes, periodic jobs does look like a pretty nice feature, but also really hard to get right, makes sense Sidekiq only has it in the commercial version.

@kraih
Copy link
Member

kraih commented Jan 18, 2016

But back to what this pull request is about, unique jobs. Why does the lock option use an arbitrary key at all? Why not just a boolean column? The name lock is also not particularly nice, something like unique => 1 would look much more appropriate.

@kraih
Copy link
Member

kraih commented Jan 18, 2016

Unique jobs in sidekiq also have a time period attached. https://github.com/mperham/sidekiq/wiki/Ent-Unique-Jobs

@avkhozov
Copy link
Contributor Author

Hmm, I thought that the user should care about what jobs need not be repeated specifying a unique key for them. How minion will know that the job should not be enqueued if the user just specify unique => 1?

The decision of sidekiq "1 job for a 1 task in a certain period" also looks like a working.

@kraih
Copy link
Member

kraih commented Jan 22, 2016

Looks like we can't reach consensus on what the right way for implementing unique jobs would be. The question about performance cost is also still unanswered. And it appears that what everyone asking for unique jobs really wants is periodic jobs, perhaps we should consider that first.

@kraih kraih closed this Jan 22, 2016
@kraih
Copy link
Member

kraih commented Jan 22, 2016

I think a feature like this is still very likely to be added in the future, but we have to look at it from all angles first, the real goal is scheduled/periodic jobs.

@kraih kraih mentioned this pull request Jan 22, 2016
@kraih
Copy link
Member

kraih commented Jan 23, 2016

Just so i don't forget to mention it in future discussions, the way the lock value is handled in retry_job is very sketchy.

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.

None yet

3 participants