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

Fix breaking change in bulk_requeue #111

Closed
wants to merge 3 commits into from
Closed

Conversation

vinc
Copy link

@vinc vinc commented Jul 28, 2020

In Sidekiq version 6.1.0 the internal method bulk_requeue became an instance method instead of a class method, this PR should fix this by creating a Sidekiq::BasicFetch instance with Sidekiq::options.

See #110 and sidekiq/sidekiq#4602

@leoarnold
Copy link

@vinc Would you mind adding a test?

@vinc
Copy link
Author

vinc commented Jul 29, 2020

That's a good idea yeah. The method isn't called anywhere directly in this gem so maybe the best would be to simply copy/adapt sidekiq's test: https://github.com/mperham/sidekiq/blob/master/test/test_fetch.rb#L39

Another thing to do I realized is to either set the minimum required version of sidekiq to 6.1.0 or to check if Sidekiq::BasicFetch class respond to the method and in that case do it the old way. I'll implement the second option I think.

@m3nd3s
Copy link

m3nd3s commented Aug 3, 2020

Hey guys! Any chance this PR to be merged?

This project has no new updates since 2017 :-/

@vinc
Copy link
Author

vinc commented Aug 4, 2020

Hey @m3nd3s, in issue #106 @brainopia expressed willingness to transfer ownership of the gem to a new maintainer but nobody answered to take up the challenge.

And I can understand that, this gems seems a bit tricky to maintain when you look at the open issues. For now I forked the gem to https://github.com/Augment/sidekiq-limit_fetch as we already have a few other gems in the same situation, but we don't currently have the required time to take proper ownership of the gem and assume a maintainer role unfortunately.

@m3nd3s
Copy link

m3nd3s commented Aug 5, 2020

OK @vinc

Probably we'll fork the repository with the fix 😄 , thank you so much.

@kreintjes
Copy link

Thanks @vinc !

@macmartine
Copy link

Is there a workaround for this, in the meantime?

@m3nd3s
Copy link

m3nd3s commented Nov 18, 2020

hey @macmartine you can fork and use the fork/repository fixed by @vinc . See #111 (comment)

But, I think it is better remove or replace the SidekiqLimitFetch from your repository in the future.

@rgaufman
Copy link

Please merge this

@stmllr
Copy link

stmllr commented Apr 13, 2021

It looks like the bulk_requeue issue has been fixed in master with #115

@vinc
Copy link
Author

vinc commented Jul 21, 2021

I'm closing this PR as it's fixed in the other merged one 👍

@vinc vinc closed this Jul 21, 2021
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

Successfully merging this pull request may close these issues.

None yet

8 participants