Add more aggressive connection pool size verification

[mperham]

Today the user can explicitly pass Sidekiq the Redis connection pool to use.

config.redis = ConnectionPool.new(size: 10) { Redis.new }

The problem is that if the concurrency is 25, the overall performance will be terrible because there aren't enough connections to handle all 25 threads concurrently. Add a check upon startup that verifies the pool size (exposed in connection_pool v2.2.2) and blows up if it is too small. For maximum performance, Sidekiq must have up to concurrency + 2 connections available.

[latortuga]

This is certainly a useful feature for new installations, however we ran into an issue this morning where an upgrade caused Sidekiq to not start after a deploy due to this change, and the route that our monitoring service used didn't detect it. The route looks like this:

require 'sidekiq/pro/web'
require 'sidekiq/api'

Rails.application.routes.draw do
  # Report whether Sidekiq latency is below 30s between queue and work.
  get "sidekiq_queue_latency" => proc { [200, {"Content-Type" => "text/plain"}, [Sidekiq::Queue.new.latency < 30 ? "OK" : "UHOH" ]] }

  # Report Sidekiq queue size below 200 jobs.
  get "sidekiq_queue_size" => proc { [200, {"Content-Type" => "text/plain"}, [Sidekiq::Queue.new.size < 200 ? "OK" : "UHOH" ]] }

Perhaps a warning is in order instead of a hard failure?

[latortuga]

Also, the change in b1bdaa7 gives me this error if pool size is specified as nil:

2018-08-13T14:27:24.783Z 20211 TID-gnwxowv57 INFO: Running in ruby 2.4.2p198 (2017-09-14 revision 59899) [x86_64-linux]
2018-08-13T14:27:24.783Z 20211 TID-gnwxowv57 INFO: Sidekiq Pro 4.0.3, commercially licensed.  Thanks for your support!
undefined method `<' for nil:NilClass
~/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/sidekiq-5.2.1/lib/sidekiq/cli.rb:87:in `run'
~/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/sidekiq-5.2.1/bin/sidekiq:12:in `<top (required)>'
bin/sidekiq:16:in `load'
bin/sidekiq:16:in `<main>'

conf = YAML.load(...) # load from config file, pool_size not present
Sidekiq.configure_server do |config|
  config.redis = ConnectionPool.new(size: conf[:pool_size]) { Redis.new(conf) }
end

[mperham]

You want this:

Sidekiq.configure_server do |config|
  config.redis = conf
end

[latortuga]

What about Sidekiq::Queue.new.latency silently reporting that everything is fine when Sidekiq is not running and jobs are being enqueued? Does Sidekiq need to be running in order for latency to be calculated?

[mperham]

I have a suspicion: if your job payload does not have an enqueued_at attribute, latency will always return 0. How are you enqueuing the jobs?

[latortuga]

ActiveJob MyJobName.perform_later, nothing changed about how we enqueue between deploys except that we upgraded from Sidekiq 4 to Sidekiq Pro 5.

[latortuga]

Actually potentially related is that our retry backoff no longer works. Could be related to enqueued_at being 0?

[mperham]

Likely unrelated. I'm not sure what's going on but the Web UI should show the current latency on the /queues page. If you shut down all Sidekiq processes and they are all zero, something's wrong.

[drock]

I agree with @latortuga that this should be a warning instead of a hard error. We just got bit by this too when upgrading. We never knew sidekiq needed the +2 connections and we have been running with a redis pool size of 25 and sidekiq thread count of 25 for years with no issues.

While we solved it by simply increasing our redis pool size, I don't think its correct to impose this restriction on everyone. Aside from the upgrade issue we experienced, there may be use cases that warrant a smaller redis pool size. It should be at the user's discretion. Warn them yes, but if they have a reason not to heed that warning, let it be.