This feature adds skip_shutdown_if option to be defined

[msxavi]

Hello everyone!

Thanks for this gem. Very useful.

I just came across an issue where I don't want to request_shutdown even though the worker exceeds max RSS. We've got, for example, a Worker class that may exceed RSS but it can run for hours. So rather than setting grace_time to hours or Float::INFINITY, I've added a skip_shutdown_if option which can receive a lambda/proc.

Sidekiq.configure_server do |config|
  Rails.logger = Sidekiq::Logging.logger

  config.server_middleware do |chain|
    max_rss = ENV['SIDEKIQ_MAX_RSS'] || 100
    chain.add Sidekiq::CustomWorkerKiller, max_rss: max_rss,
      skip_shutdown_if: proc { |worker| worker.class.name == 'LongWorker' }
  end
end

Let me know what you guys think.

Thank you in advance!

[BuonOmo]

Hi Emerson!

Thank you for your interest, it is always nice to see a new person contributing 😄

I've run through your PR and it looks alright to me. Although I have a little change requested. And it would be great if you could add your modifications to the README under Available options.

I think wouldn't use this option because exceeding max RSS means really slow performances anyway, but why not having the option I guess. @ccyrille @hugobarthelemy could one of you give his opinion as well to be sure?

Cheers
Ulysse

[BuonOmo]

I'd use something more precise to avoir users passing any other argument (a string, boolean, ...)

Suggested change

	if @skip_shutdown && @skip_shutdown.call(worker, job, queue)
	if @skip_shutdown.respond_to?(:call) && @skip_shutdown.call(worker, job, queue)

[ccyrille]

Hi Emerson,

Thanks for your contribution. From my point of view, we shouldn't multiply mechanisms to wait for very long running jobs. They are often considered a bad practise, even more if they regularly exceed RSS. The Float::INFINITY grace time is here to support these if you really want to, and it has a minimal footprint on the codebase. But I don't think we should go any further to support these cases. I am curious, why didn't you use a Float::INFINITY grace time ? Is there an important use-case we are missing here ?

Thanks again,
Cyrille

[msxavi]

@BuonOmo @ccyrille thank you guys for your time in reviewing my PR.

Firstly, I believe that exceeding max RSS can't always mean slow performances. Sometimes we just need to define a safe limit to work with and not overload important processes. I have 16 workers with a max of 5 threads each. One particular Worker must finish no matter what and it can take a while due to an extensive dataset. So we understand that the long-running process is still okay.

On the other hand, using Float::INFINITY for all the other 15 workers is a super-wide approach as they can be safely pushed back to the queue.

In these circumstances, I was aiming for a more flexible way of triggering the shutdown.

I understand your point and I'm happy if you decide this is not applicable at all.

Thanks again!

[msxavi]

I was thinking skip_shutdown_if is very specific and very dependent on the result to be a boolean.

Perhaps a less appealing and more flexible approach would be something like after_exceed callback?

  config.server_middleware do |chain|
    max_rss = ENV['SIDEKIQ_MAX_RSS'] || 100
    chain.add Sidekiq::CustomWorkerKiller, max_rss: max_rss,
      after_exceed: ->(middleware) do 
         middleware.skip_shutdown = true if middleware.worker.to_s == 'LongWorker'
      end
  end

@ccyrille Is this a little bit more attractive?

[msxavi]

As a reference https://github.com/mperham/sidekiq/wiki/API#processes

Sidekiq::ProcessSet gets you access to near real-time (updated every 5 sec)

This is to cover an issue where the process was "quieted" although, return false if process["busy"] != 0 && process["quiet"] == "true" didn't get the up-to-date information from the API and it shut down without grace_time.

[msxavi]

Moved out to #10

[ccyrille]

Hi @msxavi ,

Sorry, we have been quiet busy yesterday and couldn't get back to you.

About the "skip shutdown", we discussed it with @BuonOmo and we are ok to integrate it with these :

• you come back to your first version, which was much more simple / easy to understand and maintain, with the change requested by @BuonOmo (respond_to?(:call))
• you provide the documentation of the "skip shutdown" feature in the README

About the 5 seconds sleep fix, it seems really useful but this doesn't have much to do with the current subject. If you please can open another PR for it, it would be great !

Thanks again for your nice work 😄We would review all these ASAP.

Cyrille

[msxavi]

@ccyrille thanks! 🙏

Updated!

[ccyrille]

Thanks @msxavi for these adjustment ! It looks good to me, waiting for @BuonOmo approval before merging 😄

[BuonOmo]

[msxavi]

Thanks @ccyrille and @BuonOmo! 🎉 Please advise which release this will be available.

[ccyrille]

@msxavi released as 0.5.0 🎉

• https://github.com/klaxit/sidekiq-worker-killer/blob/master/CHANGELOG.md
• https://rubygems.org/gems/sidekiq-worker-killer

Thanks again !