-
-
Notifications
You must be signed in to change notification settings - Fork 277
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
Suggested configuration changes #328
Comments
My vote would be something like your first suggestion. Maybe something like this:
I assume this change would mean only 3 types of locks are needed? What about changing the lock names?
|
Great suggestions @tahanson ! I'm so glad I've awaited some feedback on this |
Love this idea! And I like @tahanson's suggestions for making the lock names more consistent / easier to understand I'd love the class method name to be more-obviously related to this gem, eg class ComposedWorker
include SidekiqUniqueJobs::UniqueWorker
sidekiq_options queue: :composed, retry: 0, backtrace: true
sidekiq_unique_jobs lock: :until_executing, on_conflict: :reject, expiration: 5.seconds
sidekiq_unique_jobs lock: :while_executing, on_conflict: :update, expiration: 10.seconds
end |
Been thinking a little on the way the worker is configured. There was a suggestion on how to rather than use the
UntilAndWhileExecuting
lock that two separately configured workers with different conflict strategies would be appreciated. The only two ways I can think of doing this would be something like below:They both have their advantages and drawbacks. Personally I lean towards the first suggestion which I feel provide the most intuitive way of configuring the worker. The class level
lock
method would basically just add whatever I want to the sidekiq_options hash but I like taking the gem in that direction since someone just discovered that first of all, no conflict strategy is used for theuntil_executing
part ofuntil_and_while_executing
and secondly they ran into some problems with multiple jobs being pushed.Anyone have any feedback on this? Maybe change it for v7.
The text was updated successfully, but these errors were encountered: