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.

Sign up for GitHub

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

Jump to bottom

sidekiq/util has been removed in 6.5.0 #21

Merged
merged 3 commits into from
Sep 2, 2022
Merged

sidekiq/util has been removed in 6.5.0 #21

merged 3 commits into from
Sep 2, 2022

Conversation

bf4
Copy link
Contributor

@bf4 bf4 commented Jun 8, 2022 •
edited
Loading

@bf4
Copy link
Contributor Author

bf4 commented Jun 9, 2022

I think this broke our sidekiq when I deployed it. Hold on

NoMethodError: undefined method call' for nil:NilClass (Most recent call first) Show 3 non-project frames NoMethodError: undefined method call' for nil:NilClass (Most recent call first)
Hide 4 non-project frames
File /app/vendor/bundle/ruby/3.0.0/gems/sidekiq-6.5.0/lib/sidekiq/processor.rb line 73 in rescue in run
File /app/vendor/bundle/ruby/3.0.0/gems/sidekiq-6.5.0/lib/sidekiq/processor.rb line 67 in run
File /app/vendor/bundle/ruby/3.0.0/gems/sidekiq-6.5.0/lib/sidekiq/component.rb line 8 in watchdog
File /app/vendor/bundle/ruby/3.0.0/gems/sidekiq-6.5.0/lib/sidekiq/component.rb line 17 in block in safe_thread

@bf4 bf4 force-pushed the patch-1 branch 9 times, most recently from 6849637 to 22d4704 Compare June 9, 2022 14:53
bf4
bf4 commented Jun 9, 2022
View reviewed changes
lib/sidekiq/worker_killer.rb
@@ -1,12 +1,18 @@
require "get_process_mem"
require "sidekiq"
require "sidekiq/util"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in #14 #12

xref
https://gitlab.com/gitlab-org/gitlab-foss/-/blob/48d25238c386a89e8a6af218eeb290936a8f7595/lib/gitlab/sidekiq_middleware/memory_killer.rb

@bf4 bf4 mentioned this pull request Jun 9, 2022
Closed
3 tasks
@mperham
Copy link

mperham commented Jun 9, 2022

Since worker_killer is a server middleware, you should include the new Module as noted here:

https://github.com/mperham/sidekiq/blob/main/docs/middleware.md#updated-api

You can then access the identity with config[:identity]. It looks like that's all you need sidekiq/util for.

@bf4
Copy link
Contributor Author

bf4 commented Jun 9, 2022

@mperham Thanks. Updated the PR and added a test for finding the process from the processset for the 'identity' to ensure it doesn't fail again

bf4
bf4 commented Jun 9, 2022
View reviewed changes
lib/sidekiq/worker_killer.rb
@@ -125,6 +131,10 @@ def sidekiq_process
end || raise("No sidekiq worker with identity #{identity} found")
end

def identity
config[:identity] || config["identity"]
end unless method_defined?(:identity)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include Sidekiq::Util will have defined this. Else we define it and call the interface added by Sidekiq::ServerMiddleware

bf4
bf4 commented Jun 9, 2022
View reviewed changes
spec/sidekiq/worker_killer_spec.rb
"queues" => %w[low medium high]
}]
end
it "finds the process by identity" do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is a critical failure point, I figured a test would be a good idea

@mperham
Copy link

mperham commented Jun 9, 2022

Why not just have a new version that only supports 6.5+? Let people use an old version of swk if they want to run an old version of Sidekiq.

@bf4
Copy link
Contributor Author

bf4 commented Jun 9, 2022

re "Why not just have a new version that only supports 6.5+? Let people use an old version of swk if they want to run an old version of Sidekiq."

I'm not a maintainer of this. 👍 on that, though

@bf4
Copy link
Contributor Author

bf4 commented Jun 10, 2022

Still getting failures. Gotta look deeper. maybe something about the heartbeat? maybe something redis-specific which is why it's not showing up in tests?

#21 (comment)

looks like somehow Processor is getting a nil @callback

I'm trying to reproduce it locally and get more context

my local gem changes, fwiw

sidekiq 6.4.2 -> 6.5.0
sidekiq-pro 5.3.1 -> 5.5.0

@mperham
Copy link

mperham commented Jun 10, 2022

Scout APM’s gem breaks Sidekiq 6.5 with this error, see their issue tracker.

@bf4
Copy link
Contributor Author

bf4 commented Jun 10, 2022

@mperham Thanks for that. I thought I might have noticed it on local server shutdown but hadn't reproduced it yet

scoutapp/scout_apm_ruby#449
sidekiq/sidekiq#5375

@slayer
Copy link

slayer commented Jun 16, 2022

Why not just have a new version that only supports 6.5+? Let people use an old version of swk if they want to run an old version of Sidekiq.

It's a bad idea to add breaking changes to minor update, isn't ?

@mperham
Copy link

mperham commented Jun 16, 2022 •
edited
Loading

Sidekiq::Processor is not a public API nor is sidekiq/util. I take compatibility very seriously for documented, public APIs, deprecate things and document them in major version release notes.

@slayer
Copy link

slayer commented Jun 16, 2022

Sidekiq::Processor is not a public API nor is sidekiq/util.

so why so many gems were broken? (rhetorical question) :)

@Laykou
Copy link

Laykou commented Jul 26, 2022

What needs to be done to merge this and release a new gem version, please?

@pcomans
Copy link

pcomans commented Aug 24, 2022

@slayer @bf4 is this ready to be merged? If not, how can I help?

@bf4
Copy link
Contributor Author

bf4 commented Aug 24, 2022

I'm running this fork in prod. I am not a maintainer can't merge or release

@themilkman
Copy link

@ccyrille Any chance for a new release?

ccyrille
ccyrille approved these changes Sep 2, 2022
View reviewed changes
Copy link
Contributor

@ccyrille ccyrille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bf4, thanks a lot for your contribution and sorry for the late reply! LGTM! I will release a new version of the gem today (cc @slayer @themilkman).

Please note that we are not using sidekiq-worker-killer internally at Klaxit anymore. Maybe it would be a good to add external maintainers. Is it something you could be interested in ?

@ccyrille ccyrille merged commit 207e292 into klaxit:master Sep 2, 2022
@ccyrille
Copy link
Contributor

ccyrille commented Sep 2, 2022

Released as 1.0.1 @bf4 😃

@bf4 bf4 deleted the patch-1 branch September 3, 2022 02:14
@bf4
Copy link
Contributor Author

bf4 commented Sep 3, 2022

@ccyrille Sure, I'm up to add some bandwidth in both github and rubygems, if you're up for it. Is really just in maintenance mode anyhow.

@bf4 bf4 restored the patch-1 branch September 5, 2022 04:35
@bf4 bf4 deleted the patch-1 branch November 7, 2022 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slayer slayer slayer approved these changes

@ccyrille ccyrille ccyrille approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bf4 @mperham @slayer @Laykou @pcomans @themilkman @ccyrille