Respect sidekiq_options overridden by .set #27
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello, we are currently testing v3 version of the gem with completely rewritten code, it should be released pretty soon. I'll make sure to include this fix in the release |
|
Hi @kukicola. I saw the re-write underway, and this code should fit there as well since it's only to do with how the job's options are fetched, and not really anything to do with the debouncing machinery. If there's a stable branch for v3, I can open a PR targeting that branch too. |
|
@stevenharman sorry for the very delayed response. v3 branch has been merged to the master, can you rebase? |
Sidekiq provides a `Sidekiq::Job.set(…)` API which can override `sidekiq_options` set at the class level, on a job-by-job instance basis. Sidekiq does the work of merging the options from `set` with those from the class-level `sidekiq_options`, into the "job" (really a normalized hash) that's handed to the middleware. So if we first look there for any `debounce:` options, rather than going to the `klass.get_sidekiq_options`, we can respect any overrides from the `.set` API. In this commit I left the `klass.get_sidekiq_options` as a fallback, though I'm not sure that's actually necessary. NOTE: This Sidekiq does not (currently?) deeply-merged nested options. I've asked about this elsewhere: sidekiq/sidekiq#6366
This removes two of the deprecated usages of `.debounce` in favor of `.perform_async`. It also adds an assertion for the one place we want to call the deprecated method, making sure we emit the warning message.
stevenharman
force-pushed
the
respect_set_option_overrides
branch
from
3ecd61d
to
46e6009
Compare
|
@kukicola rebased, and included another commit to fix some deprecations in the tests. |
kukicola
approved these changes
Oct 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sidekiq provides a
Sidekiq::Job.set(…)API which can overridesidekiq_optionsset at the class level, on a job-by-job instance basis. Sidekiq does the work of merging the options fromsetwith those from the class-levelsidekiq_options, into the "job" (really a normalized hash) that's handed to the middleware. So if we first look there for anydebounce:options, rather than going to theklass.get_sidekiq_options, we can respect any overrides from the.setAPI.In this commit I left the
klass.get_sidekiq_optionsas a fallback, though I'm not sure that's actually necessary.NOTE: This Sidekiq does not (currently?) deeply-merged nested options.
I've asked about this elsewhere: sidekiq/sidekiq#6366