ConnectionPool adapter

[bkeepers]

trafficstars

This is an attempt at creating a generic ConnectionPool adapter that can be composed with other adapters as discussed in https://github.com/flippercloud/flipper/pull/826#pullrequestreview-1832444441.

It just wraps each adapter method call in a pool.with { } call, but I think this mostly makes sense since the adapter API already represents atomic actions. It's not a lot of code and can be used with any adapter that requires a connection pool (Redis, RedisCache, and probably others).

ConnectionPool doesn't expose a mechanism for extending an existing pool instance, caching objects that depend on connection, or obsevering lifecycle events, so this adapter has to keep a cache of adapter instances for each connection. I'm not 100% sure this is kosher.

Usage as primary storage adapter:

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

# Read about the `config.adapter` problem below
Flipper.configure do |config|
  config.adapter { adapter }
end

Usage with cache adapter:

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

# Read about the `config.adapter` problem below
Flipper.configure do |config|
  config.adapter { adapter }
end

The config.adapter problem

Currently, Flipper returns a new adapter instance (it calls the config.adapter block) for each thread. This is to guard against some adapters not being thread safe. Initializing this adapter inside config.adapter block would probably work fine, but it would result in a separate adapter cache for each thread (n threads * x pool size).

I'm thinking we should add threadsafe? to the adapter API and change Flipper to reuse adapter instances of threadsafe adapters.

cc @cymen

[cymen]

@bkeepers Sounds good! I'm taking some time off work so I won't be able to keep my PR going forward but this sounds like a good way to go (and makes my PR not necessary).

[maleksiuk]

Thanks for working on this, @bkeepers! I started down this same path this morning and then found this PR. I'm looking forward to using it with the Redis adapter (and ideally an existing Redis pool). If it doesn't get merged for whatever reason, maybe I can contribute a less generic version for Redis.

[gstokkink]

@jnunemaker is this worth merging?

[jnunemaker]

I'm thinking we should add threadsafe? to the adapter API and change Flipper to reuse adapter instances of threadsafe adapters.

@bkeepers I like this idea.

[jnunemaker]

@bkeepers anything left on this? It seems like you've responded to all the feedback. Do we just need someone to guinea pig in production somewhere?

[gstokkink]

@jnunemaker slightly off-topic, but do you know if there are any plans to also support the new redis-client (https://github.com/redis-rb/redis-client) gem alongside the slower and more bloated redis-rb (https://github.com/redis/redis-rb) gem? Note that the redis-rb gem also uses redis-client internally, so might be more efficient to be able to cut out the middleman.

[jnunemaker]

@gstokkink sure. I'd just make a new adapter for it and specs. You can start by cloning the current redis one or taking a peek at the redis cache one. I'd just call it RedisClient.