Add RedisCacheConnectionPool adapter

[cymen]

Per conversation on issue #609 I wrote a RedisCacheConnectionPool adapter. I copied the existing tests for the RedisCache adapter and updated them. I debated testing both within the same test file however it seemed simpler to fork the test file as there are a number of differences (although not so many that it would be difficult).

This PR also adds the connection_pool gem to the Gemfile.

[bkeepers]

Thanks @cymen, the code looks good!

After seeing this, it has me wondering what it would look like to make it more generic.

1. Update RedisCache to work with Redis or ConnectionPool client

Basically define a helper that all the adapter methods call to get the client, and it just works if the given client is a ConnectionPool. Something like this:

def with_client(&block)
  @cache.respond_to?(:with) ? @cache.with(&block) : block.call(@cache)
end

Then the existing adapter methods just need to be updated to use this method:

      def add(feature)
        result = @adapter.add(feature)
-       @cache.del(@features_key)
+       with_client { |client| client.del(@features_key) }
        result
      end

If we go this route, it would be good to update the Redis and Dalli adapters to use this approach too.

2. A generic ConnectionPool adapter

Another option is to define a ConnectionPool adapter that would work with any of the other adapters that take a client. I'm not exactly sure how it would work. One option would be to have it take same args as ConnectionPool, and just return adapters instead of clients.

storage = Flipper::Adapters::ActiveRecord.new
Flipper::Adapters::ConnectionPool.new(size: 5, timeout: 5) do
  Flipper::Adapters::RedisCache.new(storage, Redis.new)
end

Although I'm guessing most people are using the pool outside of Flipper, so it would probably also need to accept an existing pool:

pool = ConnectionPool.new(size: 5, timeout: 5) { Redis.new }
storage = Flipper::Adapters::ActiveRecord.new
Flipper::Adapters::ConnectionPool.new(pool) do |client|
  Flipper::Adapters::RedisCache.new(storage, client)
end

---

I'm curious your thoughts on those options.

[cymen]

@bkeepers I debated #1 quite a bit. I think it's a good idea. The only reason I didn't do it was I didn't want to put too much effort into it in case it would be rejected and this seemed like a simpler change. I think #1 is a better experience for everyone. So I think that is the way to go. That said, I don't know if it's best done on this PR or as a follow up PR. I guess on this PR as at some point the docs need to be updated and going straight to the #1 solution would make that less effort (and no wasted effort if this new one was documented and then removed).

For #2, I don't really have a strong opinion. I do think it makes sense as the connection_pool gem is really handy in that it's agnostic to the underlying data store (at least it claims to be and does seem to be). I do like the idea of keeping it composable in your later example (where we create the connection pool outside). That seems very flexible and as a developer, it's easy to understand when reading the configuration that a connection pool is in play. I've only used the active record and the redis cache flipper classes so I'm not familiar with the rest but that pattern does seem nice to me.

[BilalBudhani]

@cymen Thank you for putting this together. I'm looking forward to using this adapter in our project once it is merged.

[cymen]

Just wanted to mention that #835 sounds good to me and that I'm taking some time off work and won't be able to keep working on this. With #835 I think it isn't needed anymore assuming that gets in. But I'll leave this as is.

[jnunemaker]

@cymen what are your thoughts on this comment I had: #839 (comment). I could whip something together or you could if you have time. Adapters stay the same and just support with or without a connection pool.

[cymen]

@jnunemaker that sounds good to me. I'm on an extended break so I'm not able to do that right now but it sounds good.