Use RAILS_MAX_THREADS for client pool size
#3807
Merged
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
This is a follow up to sidekiq#2985 (52828e4) adding similar support for the client connection pool. For Rails servers, Sidekiq is not loaded from the CLI so the prior change to support setting the concurrency via `RAILS_MAX_THREADS` is not applied to the web server process. This means for Rails servers which do not configure a custom size through an initializer they will run with the default connection pool size of 5. When the Rails server runs the initial Redis connection may be made through `Sidekiq::Client` (e.g. from [`ActiveJob::QueueAdapters::SidekiqAdapter`](https://github.com/rails/rails/blob/v5.1.5/activejob/lib/active_job/queue_adapters/sidekiq_adapter.rb#L20)). This causes the `redis_pool` to be initialized without any options, setting the pool size to the default of 5. .gem/ruby/2.5.0/gems/sidekiq-5.1.1/lib/sidekiq.rb:125:in `redis_pool' .gem/ruby/2.5.0/gems/sidekiq-5.1.1/lib/sidekiq/client.rb:42:in `initialize' .gem/ruby/2.5.0/gems/sidekiq-5.1.1/lib/sidekiq/client.rb:131:in `new' .gem/ruby/2.5.0/gems/sidekiq-5.1.1/lib/sidekiq/client.rb:131:in `push' .gem/ruby/2.5.0/gems/activejob-5.1.5/lib/active_job/queue_adapters/sidekiq_adapter.rb:20:in `enqueue' .gem/ruby/2.5.0/gems/activejob-5.1.5/lib/active_job/enqueuing.rb:51:in `block in enqueue' .gem/ruby/2.5.0/gems/activesupport-5.1.5/lib/active_support/callbacks.rb:108:in `block in run_callbacks' .gem/ruby/2.5.0/gems/activejob-5.1.5/lib/active_job/logging.rb:15:in `block (3 levels) in <module:Logging>' .gem/ruby/2.5.0/gems/activejob-5.1.5/lib/active_job/logging.rb:44:in `block in tag_logger' .gem/ruby/2.5.0/gems/activesupport-5.1.5/lib/active_support/tagged_logging.rb:69:in `block in tagged' .gem/ruby/2.5.0/gems/activesupport-5.1.5/lib/active_support/tagged_logging.rb:26:in `tagged' .gem/ruby/2.5.0/gems/activesupport-5.1.5/lib/active_support/tagged_logging.rb:69:in `tagged' .gem/ruby/2.5.0/gems/activejob-5.1.5/lib/active_job/logging.rb:44:in `tag_logger' .gem/ruby/2.5.0/gems/activejob-5.1.5/lib/active_job/logging.rb:14:in `block (2 levels) in <module:Logging>' .gem/ruby/2.5.0/gems/activesupport-5.1.5/lib/active_support/callbacks.rb:117:in `instance_exec' .gem/ruby/2.5.0/gems/activesupport-5.1.5/lib/active_support/callbacks.rb:117:in `block in run_callbacks' .gem/ruby/2.5.0/gems/activesupport-5.1.5/lib/active_support/callbacks.rb:135:in `run_callbacks' .gem/ruby/2.5.0/gems/activejob-5.1.5/lib/active_job/enqueuing.rb:47:in `enqueue' .gem/ruby/2.5.0/gems/activejob-5.1.5/lib/active_job/enqueuing.rb:18:in `perform_later' For the majority of cases, a client pool size of 5 is sufficient. However, servers which utilize a high number of threads, with large job payloads, and which may experience some network latency issues can see `Timeout::Error` crashes. This may be further exacerbated by the ~2-20x performance decrease through `ActiveJob` (sidekiq#3782). Rails addresses this general type of connection issue for the main database by suggesting that the DB pool size match the number of threads running. This change applies that logic to the default client pool size by leveraging the same environment setting; this way there's a connection available per thread. This may also have the side effect of a slight performance boost, as there is less of a chance that threads will be blocked waiting on connections. The trade-off is that there may be a memory profile increase to handle the additional Redis connections in the pool; note the pool only creates new connections as necessary to handle the requests. Resolves sidekiq#3806
|
Really solid work. 🤘 |
tahb
added a commit
to DFE-Digital/teaching-vacancies
that referenced
this pull request
Feb 28, 2019
* Everything uses 5 threads by default * We can change this setting by a variable if we really need to make tweaks (due to memory usage investigation in this case) * This will reduce Sidekiq from the default 10 concurrency to 5 which is the default active record pool size. * RAILS_MAX_THREADS variable used as the configurable variable for this. Sidekiq have done so here sidekiq/sidekiq#3807
tahb
added a commit
to DFE-Digital/teaching-vacancies
that referenced
this pull request
Mar 1, 2019
* Everything uses 5 threads by default * We can change this setting by a variable if we really need to make tweaks (due to memory usage investigation in this case) * This will reduce Sidekiq from the default 10 concurrency to 5 which is the default active record pool size. * RAILS_MAX_THREADS variable used as the configurable variable for this. Sidekiq have done so here sidekiq/sidekiq#3807
tahb
added a commit
to DFE-Digital/teaching-vacancies
that referenced
this pull request
Mar 4, 2019
* Everything uses 5 threads by default. Before this change Sidekiq was using 10 workers which was more than the 5 default active record connections. This can lead to database connection issues for Sidekiq. From our autoscaling work we know that we are comfortable with each container having 5 puma threads and 5 db connections so we need Sidekiq to follow along for stability * We can change this setting by a variable if we really need to make tweaks (due to memory usage investigation in this case), I'd suggest we update variables.tf in a more permenant change rather than through .tfvars which suggests it's safe to tweak with per each environment * This will reduce Sidekiq from the default 10 concurrency to 5 which is the default active record pool size. * RAILS_MAX_THREADS variable used as the configurable variable for this. Sidekiq have done so here sidekiq/sidekiq#3807
erbridge
pushed a commit
to DFE-Digital/teaching-vacancies
that referenced
this pull request
Apr 24, 2019
* Everything uses 5 threads by default. Before this change Sidekiq was using 10 workers which was more than the 5 default active record connections. This can lead to database connection issues for Sidekiq. From our autoscaling work we know that we are comfortable with each container having 5 puma threads and 5 db connections so we need Sidekiq to follow along for stability * We can change this setting by a variable if we really need to make tweaks (due to memory usage investigation in this case), I'd suggest we update variables.tf in a more permenant change rather than through .tfvars which suggests it's safe to tweak with per each environment * This will reduce Sidekiq from the default 10 concurrency to 5 which is the default active record pool size. * RAILS_MAX_THREADS variable used as the configurable variable for this. Sidekiq have done so here sidekiq/sidekiq#3807
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.
This is a follow up to #2985 (52828e4) adding similar support for the client connection pool. For Rails servers, Sidekiq is not loaded from the CLI so the prior change to support setting the concurrency via
RAILS_MAX_THREADSis not applied to the web server process. This means for Rails servers which do not configure a custom size through an initializer they will run with the default connection pool size of 5.When the Rails server runs the initial Redis connection may be made through
Sidekiq::Client(e.g. fromActiveJob::QueueAdapters::SidekiqAdapter). This causes theredis_poolto be initialized without any options, setting the pool size to the default of 5.For the majority of cases, a client pool size of 5 is sufficient. However, servers which utilize a high number of threads, with large job payloads, and which may experience some network latency issues can see
Timeout::Errorcrashes. This may be further exacerbated by the ~2-20x performance decrease throughActiveJob(#3782). Rails addresses this general type of connection issue for the main database by suggesting that the DB pool size match the number of threads running. This change applies that logic to the default client pool size by leveraging the same environment setting; this way there's a connection available per thread.This may also have the side effect of a slight performance boost, as there is less of a chance that threads will be blocked waiting on connections. The trade-off is that there may be a memory profile increase to handle the additional Redis connections in the pool; note the pool only creates new connections as necessary to handle the requests.
Resolves #3806