From 90808d877a28daf08a3652cd08780fbdb1ee5ff6 Mon Sep 17 00:00:00 2001 From: Brandon Keepers Date: Sat, 3 Feb 2024 07:21:00 -0500 Subject: [PATCH 1/4] Create generic conneciton pool adapter --- Gemfile | 1 + lib/flipper/adapter.rb | 14 +++++++++++ lib/flipper/adapters/connection_pool.rb | 25 +++++++++++++++++++ spec/flipper/adapters/connection_pool_spec.rb | 24 ++++++++++++++++++ 4 files changed, 64 insertions(+) create mode 100644 lib/flipper/adapters/connection_pool.rb create mode 100644 spec/flipper/adapters/connection_pool_spec.rb diff --git a/Gemfile b/Gemfile index cea81923e..01e4e67e0 100644 --- a/Gemfile +++ b/Gemfile @@ -28,6 +28,7 @@ gem 'mysql2' gem 'pg' gem 'cuprite' gem 'puma' +gem 'connection_pool' group(:guard) do gem 'guard' diff --git a/lib/flipper/adapter.rb b/lib/flipper/adapter.rb index 7e59e0502..a31dba621 100644 --- a/lib/flipper/adapter.rb +++ b/lib/flipper/adapter.rb @@ -1,6 +1,20 @@ module Flipper # Adding a module include so we have some hooks for stuff down the road module Adapter + OPERATIONS = [ + :add, + :clear, + :disable, + :enable, + :export, + :features, + :get_all, + :get_multi, + :get, + :import, + :remove, + ] + def self.included(base) base.extend(ClassMethods) end diff --git a/lib/flipper/adapters/connection_pool.rb b/lib/flipper/adapters/connection_pool.rb new file mode 100644 index 000000000..5b1714a7c --- /dev/null +++ b/lib/flipper/adapters/connection_pool.rb @@ -0,0 +1,25 @@ +# An adapter that uses ConnectionPool to manage connections. +# +# Usage: +# +# pool = ConnectionPool.new(size: 5, timeout: 5) { Redis.new } +# Flipper::Adapters::ConnectionPool.new(pool) do |client| +# Flipper::Adapters::Redis.new(client) +# end +# +class Flipper::Adapters::ConnectionPool + include Flipper::Adapter + + def initialize(pool = nil, &adapter_initializer) + @pool = pool + @adapter_initializer = adapter_initializer + end + + OPERATIONS.each do |method| + define_method(method) do |*args| + @pool.with do |connection| + @adapter_initializer.call(connection).public_send(method, *args) + end + end + end +end diff --git a/spec/flipper/adapters/connection_pool_spec.rb b/spec/flipper/adapters/connection_pool_spec.rb new file mode 100644 index 000000000..43393b5c2 --- /dev/null +++ b/spec/flipper/adapters/connection_pool_spec.rb @@ -0,0 +1,24 @@ +require "connection_pool" +require "flipper/adapters/connection_pool" +require "flipper-redis" + +RSpec.describe Flipper::Adapters::ConnectionPool do + let(:pool) do + ConnectionPool.new(size: 1, timeout: 5) do + Redis.new({url: ENV['REDIS_URL']}.compact) + end + end + + subject do + described_class.new(pool) { |client| Flipper::Adapters::Redis.new(client) } + end + + before do + skip_on_error(Redis::CannotConnectError, 'Redis not available') do + pool.with(&:flushdb) + end + end + + + it_should_behave_like 'a flipper adapter' +end From 595c15f2cdf8bb028eaec64bb84d5c02f8b9ff51 Mon Sep 17 00:00:00 2001 From: Brandon Keepers Date: Sat, 3 Feb 2024 07:58:28 -0500 Subject: [PATCH 2/4] Cache adapter instances based on client --- lib/flipper/adapters/connection_pool.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/flipper/adapters/connection_pool.rb b/lib/flipper/adapters/connection_pool.rb index 5b1714a7c..da1c52bbe 100644 --- a/lib/flipper/adapters/connection_pool.rb +++ b/lib/flipper/adapters/connection_pool.rb @@ -13,12 +13,14 @@ class Flipper::Adapters::ConnectionPool def initialize(pool = nil, &adapter_initializer) @pool = pool @adapter_initializer = adapter_initializer + @adapter_cache = {} end OPERATIONS.each do |method| define_method(method) do |*args| @pool.with do |connection| - @adapter_initializer.call(connection).public_send(method, *args) + @adapter_cache[connection] ||= @adapter_initializer.call(connection) + @adapter_cache[connection].public_send(method, *args) end end end From 8d686627b40d6602878530b1fdaed0a236709208 Mon Sep 17 00:00:00 2001 From: Brandon Keepers Date: Sat, 3 Feb 2024 09:19:18 -0500 Subject: [PATCH 3/4] Optionally create a new pool --- lib/flipper/adapters/connection_pool.rb | 31 ++++++++++++++----- spec/flipper/adapters/connection_pool_spec.rb | 29 +++++++++++------ 2 files changed, 42 insertions(+), 18 deletions(-) diff --git a/lib/flipper/adapters/connection_pool.rb b/lib/flipper/adapters/connection_pool.rb index da1c52bbe..d19157430 100644 --- a/lib/flipper/adapters/connection_pool.rb +++ b/lib/flipper/adapters/connection_pool.rb @@ -2,26 +2,41 @@ # # Usage: # +# # Reuse an existing pool to create new adapters # pool = ConnectionPool.new(size: 5, timeout: 5) { Redis.new } # Flipper::Adapters::ConnectionPool.new(pool) do |client| # Flipper::Adapters::Redis.new(client) # end # +# # Create a new pool that returns the adapter +# Flipper::Adapters::ConnectionPool.new(size: 5, timeout: 5) do +# Flipper::Adapters::Redis.new(Redis.new) +# end class Flipper::Adapters::ConnectionPool include Flipper::Adapter - def initialize(pool = nil, &adapter_initializer) - @pool = pool - @adapter_initializer = adapter_initializer - @adapter_cache = {} + # Use an existing ConnectionPool to initialize and cache new adapter instances. + class Wrapper + def initialize(pool, &initializer) + @pool = pool + @initializer = initializer + @instances = {} + end + + def with(&block) + @pool.with do |resource| + yield @instances[resource] ||= @initializer.call(resource) + end + end + end + + def initialize(options = {}, &adapter_initializer) + @pool = options.is_a?(ConnectionPool) ? Wrapper.new(options, &adapter_initializer) : ConnectionPool.new(options, &adapter_initializer) end OPERATIONS.each do |method| define_method(method) do |*args| - @pool.with do |connection| - @adapter_cache[connection] ||= @adapter_initializer.call(connection) - @adapter_cache[connection].public_send(method, *args) - end + @pool.with { |adapter| adapter.public_send(method, *args) } end end end diff --git a/spec/flipper/adapters/connection_pool_spec.rb b/spec/flipper/adapters/connection_pool_spec.rb index 43393b5c2..430a35023 100644 --- a/spec/flipper/adapters/connection_pool_spec.rb +++ b/spec/flipper/adapters/connection_pool_spec.rb @@ -3,22 +3,31 @@ require "flipper-redis" RSpec.describe Flipper::Adapters::ConnectionPool do - let(:pool) do - ConnectionPool.new(size: 1, timeout: 5) do - Redis.new({url: ENV['REDIS_URL']}.compact) + let(:redis_options) { {url: ENV['REDIS_URL']}.compact } + + before do + skip_on_error(Redis::CannotConnectError, 'Redis not available') do + Redis.new(redis_options).flushdb end end - subject do - described_class.new(pool) { |client| Flipper::Adapters::Redis.new(client) } - end + context "with an existing pool" do + let(:pool) do + ConnectionPool.new(size: 1, timeout: 5) { Redis.new(redis_options) } + end - before do - skip_on_error(Redis::CannotConnectError, 'Redis not available') do - pool.with(&:flushdb) + subject do + described_class.new(pool) { |client| Flipper::Adapters::Redis.new(client) } end + + it_should_behave_like 'a flipper adapter' end + context "with a new pool" do + subject do + described_class.new(size: 2) { Flipper::Adapters::Redis.new(Redis.new(redis_options)) } + end - it_should_behave_like 'a flipper adapter' + it_should_behave_like 'a flipper adapter' + end end From 18b2a1bcbd6352a8d336a10d5dfa206e4af7f928 Mon Sep 17 00:00:00 2001 From: Brandon Keepers Date: Thu, 15 Feb 2024 09:57:06 -0500 Subject: [PATCH 4/4] Add Observable to ConnectionPool --- lib/flipper/adapters/connection_pool.rb | 34 +++++++++++++++++++ spec/flipper/adapters/connection_pool_spec.rb | 7 ++++ 2 files changed, 41 insertions(+) diff --git a/lib/flipper/adapters/connection_pool.rb b/lib/flipper/adapters/connection_pool.rb index d19157430..7d0a9bea6 100644 --- a/lib/flipper/adapters/connection_pool.rb +++ b/lib/flipper/adapters/connection_pool.rb @@ -1,3 +1,28 @@ +require "connection_pool" +require "observer" + +# Make ConnectionPool observable so we can reset the cache when the pool is reloaded or shutdown +ConnectionPool.class_eval do + module ShutdownObservable + def reload(&block) + super(&block).tap do + changed + notify_observers + end + end + + def shutdown(&block) + super(&block).tap do + changed + notify_observers + end + end + end + + include Observable + prepend ShutdownObservable +end + # An adapter that uses ConnectionPool to manage connections. # # Usage: @@ -21,6 +46,9 @@ def initialize(pool, &initializer) @pool = pool @initializer = initializer @instances = {} + + # Reset the cache when the pool is reloaded or shutdown + @pool.add_observer(self, :reset) end def with(&block) @@ -28,8 +56,14 @@ def with(&block) yield @instances[resource] ||= @initializer.call(resource) end end + + def reset + @instances.clear + end end + attr_reader :pool + def initialize(options = {}, &adapter_initializer) @pool = options.is_a?(ConnectionPool) ? Wrapper.new(options, &adapter_initializer) : ConnectionPool.new(options, &adapter_initializer) end diff --git a/spec/flipper/adapters/connection_pool_spec.rb b/spec/flipper/adapters/connection_pool_spec.rb index 430a35023..e64c88764 100644 --- a/spec/flipper/adapters/connection_pool_spec.rb +++ b/spec/flipper/adapters/connection_pool_spec.rb @@ -21,6 +21,13 @@ end it_should_behave_like 'a flipper adapter' + + it "should reset the cache when the pool is reloaded or shutdown" do + subject.get_all + expect { pool.reload { |_| } }.to change { subject.pool.instance_variable_get(:@instances).size }.from(1).to(0) + subject.get_all + expect { pool.shutdown { |_| } }.to change { subject.pool.instance_variable_get(:@instances).size }.from(1).to(0) + end end context "with a new pool" do