mhenrixon / sidekiq-unique-jobs

Prevents duplicate Sidekiq jobs
MIT License
1.47k stars 277 forks source link

Conflicts with sidekiq-lock gem #839

Open sdhull opened 7 months ago

sdhull commented 7 months ago

Describe the bug Both sidekiq-lock and sidekiq-uniq-jobs use the :lock key in sidekiq_options. This makes them impossible (or dangerous?) to use simultaneously

Expected behavior Be able to use both of these gems simultaneously. I think that this gem provides all the functionality of sidekiq-lock and more, but I'm not sure I want to take on the additional effort of converting existing jobs to use this... especially considering I'm not sure I could make that change without breaking the locking behavior of existing jobs, at least during the switch.

Current behavior sidekiq_options keys conflicting with other gem

Additional context It would be great if sidekiq-unique-jobs would nest all options under a unique key in sidekiq_options. I can't help but wonder if sidekiq gems could adopt a convention to name their option key after their gem, eg sidekiq_options unique_jobs: { lock: :until_executed, etc }... Or possibly offer a configuration option to configure the key used in sidekiq_options.

Any guidance here would be appreciated. Would you be interested in a PR to introduce a configuration option for sidekiq_options key?

mhenrixon commented 7 months ago

Any guidance here would be appreciated. Would you be interested in a PR to introduce a configuration option for sidekiq_options key?

Not interested in that. The thing is, you could easily add your own simple custom lock: see https://github.com/mhenrixon/sidekiq-unique-jobs?tab=readme-ov-file#custom-locks

# lib/locks/my_custom_lock.rb
module Locks
  class MyCustomLock < SidekiqUniqueJobs::Lock::BaseLock
    def execute
      # Do something ...
    end
  end
end

SidekiqUniqueJobs.configure do |config|
  config.add_lock :my_custom_lock, Locks::MyCustomLock
end

Adding support for having another lock gem is not something I am willing to entertain.

sdhull commented 7 months ago

I'm not sure how defining a custom lock addresses the issue.

The issue is that every key introduced to sidekiq_options is an opportunity to collide with other gems and in fact sidekiq itself. An easy way to minimize risk of collision is to nest options under a single key.

Anyway I'll close this as I think you've answered my question; the fact that I'm not super thrilled with the answer is irrelevant 😭

mhenrixon commented 7 months ago

@sdhull, maybe I was a little biased against your viewpoint, but nesting the keys is a discussion I already had and wanted to have.

I was thinking something more like this originally: https://github.com/mhenrixon/sidekiq-unique-jobs/issues/328

If you want to look at this, what would it look like?

I'm sorry; it was intense last year, and I didn't even really take the time to read the issue properly. I saw sidekiq-lock and shut down :)

sdhull commented 7 months ago

@mhenrixon aww thanks for continuing the convo! ❤️ and sorry to hear life has been intense—I know how it can be ❤️‍🩹

I love the idea of introducing a new class method to handle configuration for this gem! I'll add a comment to #328