Fetch API refactor, WIP #4602
Merged
Fetch API refactor, WIP #4602
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
mperham
pushed a commit
that referenced
this pull request
Jul 1, 2020
Withing #4602 this method was changed from a class- to instance-method. The comment should be removed as the method is not a class-method anymore.
|
It would be ideal if either a class or instance could be passed, for backwards compatibility with 3rd party gems. |
|
Shouldn't this have been a major version change as it's not backwards compatible? I didn't expect a version change from 6.1.0 to 6.1.1 to break anything. A fix as suggested by @jhoffner to be released in version 6.1.2 would be great! |
|
The Fetch API has never been documented, it's not intended as a public, "user-facing" API. Internals can change. Furthermore I want to point out that Sidekiq is not semantic versioned. I do my best to limit any possible breaking change to minor or major releases based on the expected impact, patch releases try not to break anything. |
|
Thanks for the explanation, got it 👍 |
anero
added a commit
to controlshift/sidekiq-rate-limiter
that referenced
this pull request
Oct 7, 2020
….0 that is not backwards compatible. As part of this refactoring the object in the :fetch option is expected to be an instance of Sidekiq::BasicFetch or an object that responds to the same methods. This change allows using the gem with newer versions while keeping compatibility with old ones. See sidekiq/sidekiq#4602 for more details.
anero
added a commit
to controlshift/sidekiq-rate-limiter
that referenced
this pull request
Oct 7, 2020
….0 that is not backwards compatible. As part of this refactoring the object in the :fetch option is expected to be an instance of Sidekiq::BasicFetch or an object that responds to the same methods. This change allows using the gem with newer versions while keeping compatibility with old ones. See sidekiq/sidekiq#4602 for more details.
anero
added a commit
to controlshift/sidekiq-rate-limiter
that referenced
this pull request
Oct 7, 2020
….0 that is not backwards compatible. As part of this refactoring the object in the :fetch option is expected to be an instance of Sidekiq::BasicFetch or an object that responds to the same methods. This change allows using the gem with newer versions while keeping compatibility with old ones. See sidekiq/sidekiq#4602 for more details.
This was referenced Oct 7, 2020
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.
The existing fetch API grew organically to its current state but is less than optimal: you pass a class reference via the global options, making it hard to configure a fetcher instance and requiring some test gymnastics to set up that global config.
I'd like to change the fetch API to be instance-based.
bulk_requeuewill become an instance method. Feedback welcome.