Skip to content
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

Connection pool support #609

Closed
jose-zenledger opened this issue Feb 22, 2022 · 10 comments
Closed

Connection pool support #609

jose-zenledger opened this issue Feb 22, 2022 · 10 comments

Comments

@jose-zenledger
Copy link

Instead of a new connection for each flipper instance, it would be nice to use connection pooling.

In config/initializers/redis.rb we have:

$redis = ConnectionPool.new(size: [ENV['REDIS_POOL'].to_i, 1].max) { Redis.new(url: ENV['REDIS_URL'])  }

in config/initializers/flipper.rb we want to do the following:

require 'flipper'
require 'flipper/adapters/redis'
require 'flipper/adapters/active_support_cache_store'

Flipper.configure do |config|
  config.adapter do
    Flipper::Adapters::ActiveSupportCacheStore.new(
      Flipper::Adapters::Redis.new($redis),
      ActiveSupport::Cache::MemoryStore.new,
      expires_in: 10.seconds
    )
  end
end

However this fails when using flipper since the methods required for redis are not defined on the connection pool. To access the redis methods you have to do:

$redis.with do |redis|
  # do operations on redis
end

Do you think we can add support? I'm happy to work on it

@jose-zenledger
Copy link
Author

I have this to proxy methods through a connection pool but it is not as optimal as native support:

class ConnectionPoolProxy
  def initialize(pool)
    @pool = pool
    @instance_class = pool.with(&:class)
  end

  def method_missing(method_name, *args, &block)
    @pool.with { |instance| instance.public_send(method_name, *args, &block) }
  end

  def respond_to_missing?(method_name, _ = false)
    @instance_class.public_instance_methods.include?(method_name.to_sym)
  end
end

@jnunemaker
Copy link
Collaborator

You can also just make a new flipper adapter. That said, I'm cool with a RedisConnectionPool adapter in flipper. Seems good to me.

@jnunemaker
Copy link
Collaborator

I'm going to close this issue to keep things tidy but if you have any more questions let me know. Happy to merge a PR for a connection pool adapter.

@jnunemaker
Copy link
Collaborator

Also if I wasn't clear before, which re-reading I'm wondering if I was, what I meant is we could add RedisConnectionPool adapter in flipper. I'd start by copying the redis one and then adding the necessary pool.with blocks around accessing redis.

@cymen
Copy link

cymen commented Jan 16, 2024

@jnunemaker Did anything ever come of this? It would be really great to have an adapter that use the Redis connection pool.

@cymen
Copy link

cymen commented Jan 16, 2024

It would also be great to have connection pool support for Redis in the RedisCache adapter.

@bkeepers
Copy link
Collaborator

@cymen no, but we'd gladly accept a pull request that adds a Redis connection pool adapter.

@cymen
Copy link

cymen commented Jan 18, 2024

@bkeepers sounds good -- I wrote one and opened a PR here #826

@j-castellanos
Copy link

@bkeepers I came up with a solution as well #839

@jeffbax
Copy link

jeffbax commented Feb 29, 2024

Just wanted to say that I would love to see one of these solutions make it in :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants