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

Make redis and redis cache work with connection pool #910

Merged
merged 5 commits into from
Feb 19, 2025

Conversation

jnunemaker
Copy link
Collaborator

This makes it so redis and redis cache work with a redis instance or a connection pool instance that generates redis instances.

@cymen @j-castellanos @maleksiuk @BilalBudhani @tanelsuurhans @gstokkink Any thoughts on this? Seems like it'll just work.

refs #826, #839, #835

To Use

All you need to do is configure your adapter:

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

If you're using redis cache, it's the same thing but pass connetion pool instance for cache instead of redis instance.

It was trying to use the redis adapter instead of the redis-rb stuff.

This forces it to the top namespace.
Might as well add default just to make it easier for people to get going.
@jnunemaker jnunemaker self-assigned this Feb 19, 2025
@jnunemaker jnunemaker merged commit cc9a158 into main Feb 19, 2025
92 checks passed
@jnunemaker jnunemaker deleted the redis-connection-pool branch February 19, 2025 14:44
@gstokkink
Copy link

@jnunemaker nice, thanks! Any chance of a new release soonish?

@jnunemaker
Copy link
Collaborator Author

Yes. Trying to get a couple more fixes in now.

@j-castellanos
Copy link

@jnunemaker great news! Keep us posted so we can update and then I can remove this code that I have been using in the meantime:

module Flipper
   module Adapters
     module Poolable
       def initialize(client_or_pool = nil, key_prefix: nil)
         @pool = nil
         @client = nil
         if client_or_pool.is_a?(ConnectionPool)
           @pool = client_or_pool
         else
           @client = client_or_pool
         end
         @key_prefix = key_prefix
       end

       def self.included(klass)
         klass.superclass.instance_methods(false).each do |method|
           klass.define_method method do |*args|
             return super(*args) unless @client.nil?

             @pool.with do |client|
               @client = client
               super(*args)
             ensure
               @client = nil
             end
           end
         end
       end
     end
   end
 end

Just out of curiosity, have people requested pooling for adapter other than Redis? If so you can probably just make clients for all adapters a pool with a default size of 1 to mimic current behavior.

@maleksiuk
Copy link

@jnunemaker This looks great! I appreciate the straightforward approach and look forward to using it. Thanks for working on this.

@BilalBudhani
Copy link

looks nice and clean. I have created a todo in our internally to use this adapter. I will report back once I have enough data. Thank you @jnunemaker!

@gstokkink
Copy link

About to use this in production 😄

@jnunemaker
Copy link
Collaborator Author

🔥 in the 🕳️ !!!

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

Successfully merging this pull request may close these issues.

5 participants