Limit number of redis connections and fix ttl bug #75
Conversation
|
Hey @anirbanmu. Thanks a bunch for the contribution :) Looks like CI will need some fixing due to bundler upgrade. |
| @@ -1,9 +1,22 @@ | |||
| require 'connection_pool' | |||
| require 'redis' | |||
| require 'concurrent-ruby' | |||
There was a problem hiding this comment.
Now that we explicitly use concurrent ruby we should add dependency to .gemspec
| @@ -1,9 +1,22 @@ | |||
| require 'connection_pool' | |||
There was a problem hiding this comment.
This can be removed as a dependency now
|
General question: instead of reimplementing connnections cache wouldn't it make more sense to cache connection pool? |
Good question. Basically, in my understanding, a connection pool only makes sense if one wants to have a pool of connections that will be shared by multiple consumers. In the design I went with, there is one connection per ruby thread, so there are never multiple consumers for the cached connection. So with the one per thread design, I don't think having a connection pool makes sense. I think caching a connection pool might make sense, if we changed the design to have a per process connection pool which is then shared by all threads in that process or something like that. That may be a direction worth exploring but I like the cleanliness of not having to worry about a global pool/sharing, and still getting a decrease in # of redis connections. PS - I fixed travis CI & updated the gemspec as well. |
…edis connection. With this design, each thread has its own redis connection (which is replaced if for some reason a new redis url is seen (should be unlikely))
…doesn't exist in redis). Added tests to verify as well.
anirbanmu
force-pushed
the
limit-redis-conn-and-fix-ttl-bug
branch
from
0dc4643
to
2f0ebe7
Compare
anirbanmu
force-pushed
the
limit-redis-conn-and-fix-ttl-bug
branch
from
2f0ebe7
to
05a538a
Compare
|
@anirbanmu thanks for the detailed explanation. I agree now that Connection Pooling might wait :) |
|
Now time to update the docs and config about connection pools, thank you for your contribution! <3 |
clientwith a memoizedClient.newwould create a new redis connection. This was causing a lot of redis connections for us & causing us to hit the limit of our redis instance. With commit a94719b, we now hold only open redis connection per thread via a thread local class variable. This lowers the amount of redis connection substantially.