-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Deprecate :pool_size
and :pool_timeout
options for configuring connection pooling in cache stores
#45111
Conversation
# One of the three threads will fail in 1 second because our pool size | ||
# is only two. | ||
3.times do | ||
threads << Thread.new do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need such a complicated test, I also fear it will be flaky.
I'd recommend to just check wether pooling is enabled, you can use instance_variable_get
or send
if necessary to check the internal state of the store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just replicated the test above 😄 Updated with the suggestion.
|
||
```ruby | ||
config.cache_store = :mem_cache_store, "cache.example.com", { pool_size: 5, pool_timeout: 5 } | ||
config.cache_store = :mem_cache_store, "cache.example.com", pool: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should really make pooling the default. Most people deploy Rails in a threaded environment now, and having a single redis connection can be a major contention point.
Can be a followup though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, but I agree, it probably would be better as a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@casperisfine When enabling pooling by default, how do you think we need to make connection_pool
a requirement? I am not sure making connection_pool
a dependency of active_support
is a good idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fatkodima, no we can just display an error just like if you use mem_cache_store
and are missing dalli
.
…nnection pooling in cache stores
adf3bf3
to
059d1d1
Compare
Fixes #39479.
cc @byroot