Compatibility for different Sidekiq uniqueness options#5
Compatibility for different Sidekiq uniqueness options#5dirk wants to merge 7 commits intoEverlane:masterfrom
Conversation
tiagomjorge
left a comment
tiagomjorge
left a comment
There was a problem hiding this comment.
Thanks a lot for taking the time to improve this awesome gem, @dirk! I have only a small suggestion which I'd love to hear your thoughts about.
lib/async_cache/workers/sidekiq.rb
Outdated
| module Options | ||
| def self.included(mod) | ||
| if defined?(Sidekiq::Enterprise) | ||
| mod.sidekiq_options unique_for: 10.minutes |
There was a problem hiding this comment.
These 10.minutes seem quite arbitrary. Wouldn't it be better to have it living in a config somewhere?
| # Only allow one job per set of arguments to ever be in the queue | ||
| sidekiq_options :unique => :until_executed | ||
| # Pulled out into a module so it can be tested. | ||
| module Options |
lib/async_cache/workers/sidekiq.rb
Outdated
| module Options | ||
| def self.included(mod) | ||
| if defined?(Sidekiq::Enterprise) | ||
| mod.sidekiq_options unique_for: 10.minutes |
There was a problem hiding this comment.
Also, those behaviors seem quite different:
until_executed if we are using the sidekiq-unique-jobs gem
VS
for 10 minutes if we are using the sidekiq enterprise gem.
This is one more reason why I believe that this time should live in a config somewhere and be very well documented.
There was a problem hiding this comment.
@tiagomjorge these behaviors are more similar than the naming implies. The Sidekiq Enterprise implementation would be equivalent to something like until_executed_or_timeout. The lock is released whenever the first of these two is satisfied, which is usually execution.
To make these two effectively the same we can set the unique_for to an arbitrarily long amount of time. 10 minutes might not be long enough for all situations though, so I agree that a documented config would be nice.
@tiagomjorge: Sounds totally reasonable! Any preferences for how that configuration should be defined/provided? |
|
@dirk we could use an initializer, i.e. |
|
@tiagomjorge: I'll look into that approach! 😄 |
|
a wild dirk appears |
...and breaks CI. 😂 |
|
@tiagomjorge, @taylorlapeyre: CI is all happy now. Mind doing another review? 🙃 (I would assign back to Tiago but I don't got no permissions anymore. 🤣) |
4 participants
Makes the Sidekiq worker work with both the
sidekiq-unique-jobsgem and Sidekiq Enterprise.Fixes #4.