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

Conflicts with sidekiq-lock gem #839

Open
sdhull opened this issue Mar 6, 2024 · 4 comments
Open

Conflicts with sidekiq-lock gem #839

sdhull opened this issue Mar 6, 2024 · 4 comments

Comments

@sdhull
Copy link

sdhull commented Mar 6, 2024

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
Copy link
Owner

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
Copy link
Author

sdhull commented Mar 6, 2024

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 😭

@sdhull sdhull closed this as completed Mar 6, 2024
@mhenrixon mhenrixon reopened this Mar 9, 2024
@mhenrixon
Copy link
Owner

@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: #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
Copy link
Author

sdhull commented Mar 12, 2024

@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

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

No branches or pull requests

2 participants