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

RAILS_MAX_THREADS does not affect client redis pool configuration #5886

Closed
krasnoukhov opened this issue Apr 20, 2023 · 6 comments
Closed

RAILS_MAX_THREADS does not affect client redis pool configuration #5886

krasnoukhov opened this issue Apr 20, 2023 · 6 comments

Comments

@krasnoukhov
Copy link
Contributor

Ruby version: 2.7.7
Rails version: 6.1.7.3
Sidekiq / Pro / Enterprise version(s): 7.0.9 / 7.0.10 / 7.0.8

Seems like it's recommended to use RAILS_MAX_THREADS to configure concurrency since Sidekiq 7.x. It works quite fine for sidekiq server: everything is set to use the same variable (AR pool, Sidekiq's redis pool, Sidekiq's thread number). However it seems like when Sidekiq is in client mode this variable is ignored for its redis pool configuration and defaults for 5. So if you have more than 5 puma threads Sidekiq client will still use pool of 5 redis connections, which leads to errors. Thanks to the fix in #5702 one could still configure it through size option but that seems redundant in case when RAILS_MAX_THREADS is already provided.

Can be reproduced like this:

RAILS_MAX_THREADS=7 rails c
Sidekiq.redis_pool.size # returns 5, expected 7
@krasnoukhov
Copy link
Contributor Author

I can try and submit a patch if my understanding is correct and this should be addressed.

@mperham
Copy link
Collaborator

mperham commented Apr 20, 2023

RAILS_MAX_THREADS is used in the Sidekiq processes to configure the default capsule concurrency. In other processes, where it is not clear what it is configuring and how that maps to the Redis pool, Sidekiq does not use it. You'd need to do this:

Sidekiq.configure_client do |config|
  config.redis = { size: Integer(ENV["RAILS_MAX_THREADS"] || 5) }
end

@krasnoukhov krasnoukhov closed this as not planned Won't fix, can't repro, duplicate, stale Apr 20, 2023
@krasnoukhov
Copy link
Contributor Author

I understand. However I still think that defaulting the pool size to RAILS_MAX_THREADS in client mode is a good idea, given that this variable is highly likely set to configure puma threads, so it would only make sense to provide compatible pool. This would probably prevent lots of wtf situations for less experienced people.

@swrobel
Copy link

swrobel commented May 2, 2023

I'm reading this and trying to make sense of what is meant here:

In this statement, is "Sidekiq processes" only referring to "server" processes?

RAILS_MAX_THREADS is used in the Sidekiq processes to configure the default capsule concurrency.

Does "In other processes" mean Rails server processes? If so, this statement strikes me as contradictory, because Sidekiq has migrated towards having a lot of automatic config based on a Rails env var that has a very clearly defined meaning.

In other processes, where it is not clear what it is configuring and how that maps to the Redis pool, Sidekiq does not use it.

@nateberkopec
Copy link
Collaborator

In this statement, is "Sidekiq processes" only referring to "server" processes?

Yes.

Does "In other processes" mean Rails server processes?

It means the Sidekiq client object inside the Rails server process.

mperham added a commit that referenced this issue May 2, 2023
@mperham
Copy link
Collaborator

mperham commented May 2, 2023

Since the pool is lazy, there's little reason not to allow more headroom so it's there if necessary. I've raised the max size from 5 to 10, will be in 7.1.1.

stanhu added a commit to stanhu/sidekiq that referenced this issue Nov 28, 2023
* sidekiq#5886 (comment)
  updated the internal pool size from 5 to 10.

* Prior to Sidekiq 7.0, `concurrency + 5` was used:
https://github.com/sidekiq/sidekiq/blame/v6.5.12/lib/sidekiq/redis_connection.rb#L93
mperham pushed a commit that referenced this issue Nov 28, 2023
* #5886 (comment)
  updated the internal pool size from 5 to 10.

* Prior to Sidekiq 7.0, `concurrency + 5` was used:
https://github.com/sidekiq/sidekiq/blame/v6.5.12/lib/sidekiq/redis_connection.rb#L93
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

No branches or pull requests

4 participants