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.
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.
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.
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.
Makes sense, but I agree, it probably would be better as a followup.
There was a problem hiding this comment.
@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.
@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
fatkodima
force-pushed
the
new-cache-pool-options
branch
from
adf3bf3
to
059d1d1
Compare
Fixes #39479.
cc @byroot