From ca8de5bc2b50938930c5e433469c97f781bd50af Mon Sep 17 00:00:00 2001 From: Cymen Vig Date: Wed, 17 Jan 2024 19:45:20 -0500 Subject: [PATCH 1/4] add connection_pool gem --- Gemfile | 1 + 1 file changed, 1 insertion(+) 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' From 28feae97e427209abc065d46e002a9130e9bd715 Mon Sep 17 00:00:00 2001 From: Cymen Vig Date: Wed, 17 Jan 2024 19:45:36 -0500 Subject: [PATCH 2/4] add RedisCacheConnectionPool adapter with tests --- .../adapters/redis_cache_connection_pool.rb | 168 ++++++++++++++++++ .../redis_cache_connection_pool_spec.rb | 106 +++++++++++ 2 files changed, 274 insertions(+) create mode 100644 lib/flipper/adapters/redis_cache_connection_pool.rb create mode 100644 spec/flipper/adapters/redis_cache_connection_pool_spec.rb diff --git a/lib/flipper/adapters/redis_cache_connection_pool.rb b/lib/flipper/adapters/redis_cache_connection_pool.rb new file mode 100644 index 000000000..3e6282b93 --- /dev/null +++ b/lib/flipper/adapters/redis_cache_connection_pool.rb @@ -0,0 +1,168 @@ +# frozen_string_literal: true +# typed: false + +require "redis" +require "flipper" + +module Flipper + module Adapters + # Public: Adapter that wraps another adapter with the ability to cache + # adapter calls in Redis + class RedisCacheConnectionPool + include ::Flipper::Adapter + + # Internal + attr_reader :pool + + # Public + def initialize(adapter, pool, ttl = 3600) + @adapter = adapter + @pool = pool + @ttl = ttl + + @version = "v1" + @namespace = "flipper/#{@version}" + @features_key = "#{@namespace}/features" + @get_all_key = "#{@namespace}/get_all" + end + + # Public + def features + read_feature_keys + end + + # Public + def add(feature) + result = @adapter.add(feature) + @pool.with do |conn| + conn.del(@features_key) + end + result + end + + # Public + def remove(feature) + result = @adapter.remove(feature) + @pool.with do |conn| + conn.del(@features_key) + conn.del(key_for(feature.key)) + end + result + end + + # Public + def clear(feature) + result = @adapter.clear(feature) + @pool.with do |conn| + conn.del(key_for(feature.key)) + end + result + end + + # Public + def get(feature) + fetch(key_for(feature.key)) do + @adapter.get(feature) + end + end + + def get_multi(features) + read_many_features(features) + end + + def get_all + @pool.with do |conn| + if conn.setnx(@get_all_key, Time.now.to_i) + conn.expire(@get_all_key, @ttl) + response = @adapter.get_all + response.each do |key, value| + set_with_ttl key_for(key), value + end + set_with_ttl @features_key, response.keys.to_set + response + else + features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) } + read_many_features(features) + end + end + end + + # Public + def enable(feature, gate, thing) + result = @adapter.enable(feature, gate, thing) + @pool.with do |conn| + conn.del(key_for(feature.key)) + end + result + end + + # Public + def disable(feature, gate, thing) + result = @adapter.disable(feature, gate, thing) + @pool.with do |conn| + conn.del(key_for(feature.key)) + end + result + end + + private + def key_for(key) + "#{@namespace}/feature/#{key}" + end + + def read_feature_keys + fetch(@features_key) { @adapter.features } + end + + def read_many_features(features) + keys = features.map(&:key) + cache_result = Hash[keys.zip(multi_cache_get(keys))] + uncached_features = features.reject { |feature| cache_result[feature.key] } + + if uncached_features.any? + response = @adapter.get_multi(uncached_features) + response.each do |key, value| + set_with_ttl(key_for(key), value) + cache_result[key] = value + end + end + + result = {} + features.each do |feature| + result[feature.key] = cache_result[feature.key] + end + result + end + + def fetch(cache_key) + cached = @pool.with do |conn| + conn.get(cache_key) + end + if cached + Marshal.load(cached) + else + to_cache = yield + set_with_ttl(cache_key, to_cache) + to_cache + end + end + + def set_with_ttl(key, value) + @pool.with do |conn| + conn.setex(key, @ttl, Marshal.dump(value)) + end + end + + def multi_cache_get(keys) + return [] if keys.empty? + + cache_keys = keys.map { |key| key_for(key) } + @pool.with do |conn| + conn.mget(*cache_keys).map do |value| + value ? Marshal.load(value) : nil + end + end + end + end + end +end diff --git a/spec/flipper/adapters/redis_cache_connection_pool_spec.rb b/spec/flipper/adapters/redis_cache_connection_pool_spec.rb new file mode 100644 index 000000000..8459ed280 --- /dev/null +++ b/spec/flipper/adapters/redis_cache_connection_pool_spec.rb @@ -0,0 +1,106 @@ +require 'flipper/adapters/operation_logger' +require 'flipper/adapters/redis_cache_connection_pool' +require 'connection_pool' + +RSpec.describe Flipper::Adapters::RedisCacheConnectionPool do + let(:client) do + options = {} + options[:url] = ENV['REDIS_URL'] if ENV['REDIS_URL'] + redis = Redis.new(options) + ConnectionPool.new(size: 5, timeout: 1) { redis } + end + + let(:memory_adapter) do + Flipper::Adapters::OperationLogger.new(Flipper::Adapters::Memory.new) + end + let(:adapter) { described_class.new(memory_adapter, client) } + let(:flipper) { Flipper.new(adapter) } + + subject { adapter } + + before do + skip_on_error(Redis::CannotConnectError, 'Redis not available') do + client.with { |conn| conn.flushdb } + end + end + + it_should_behave_like 'a flipper adapter' + + describe '#remove' do + it 'expires feature' do + feature = flipper[:stats] + adapter.get(feature) + adapter.remove(feature) + expect(client.with { |conn| conn.get("flipper/v1/feature/#{feature.key}") }).to be(nil) + end + end + + describe '#get' do + it 'uses correct cache key' do + stats = flipper[:stats] + adapter.get(stats) + expect(client.with { |conn| conn.get("flipper/v1/feature/#{stats.key}") }).not_to be_nil + end + end + + describe '#get_multi' do + it 'warms uncached features' do + stats = flipper[:stats] + search = flipper[:search] + other = flipper[:other] + stats.enable + search.enable + + memory_adapter.reset + + adapter.get(stats) + expect(client.with { |conn| conn.get("flipper/v1/feature/#{search.key}") }).to be(nil) + expect(client.with { |conn| conn.get("flipper/v1/feature/#{other.key}") }).to be(nil) + + adapter.get_multi([stats, search, other]) + + search_cache_value, other_cache_value = [search, other].map do |f| + Marshal.load(client.with { |conn| conn.get("flipper/v1/feature/#{f.key}") }) + end + expect(search_cache_value[:boolean]).to eq('true') + expect(other_cache_value[:boolean]).to be(nil) + + adapter.get_multi([stats, search, other]) + adapter.get_multi([stats, search, other]) + expect(memory_adapter.count(:get_multi)).to eq(1) + end + end + + describe '#get_all' do + let(:stats) { flipper[:stats] } + let(:search) { flipper[:search] } + + before do + stats.enable + search.add + end + + it 'warms all features' do + adapter.get_all + expect(Marshal.load(client.with { |conn| conn.get("flipper/v1/feature/#{stats.key}") })[:boolean]).to eq('true') + expect(Marshal.load(client.with { |conn| conn.get("flipper/v1/feature/#{search.key}") })[:boolean]).to be(nil) + expect(client.with { |conn| conn.get("flipper/v1/get_all").to_i }).to be_within(2).of(Time.now.to_i) + end + + it 'returns same result when already cached' do + expect(adapter.get_all).to eq(adapter.get_all) + end + + it 'only invokes one call to wrapped adapter' do + memory_adapter.reset + 5.times { adapter.get_all } + expect(memory_adapter.count(:get_all)).to eq(1) + end + end + + describe '#name' do + it 'is redis_cache_connection_pool' do + expect(subject.name).to be(:redis_cache_connection_pool) + end + end +end From a5e83ffcae5d3e19a5c8d07d3f188141f6907263 Mon Sep 17 00:00:00 2001 From: Cymen Vig Date: Wed, 17 Jan 2024 19:49:04 -0500 Subject: [PATCH 3/4] minor cleanup --- lib/flipper/adapters/redis_cache_connection_pool.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/flipper/adapters/redis_cache_connection_pool.rb b/lib/flipper/adapters/redis_cache_connection_pool.rb index 3e6282b93..cc8471c30 100644 --- a/lib/flipper/adapters/redis_cache_connection_pool.rb +++ b/lib/flipper/adapters/redis_cache_connection_pool.rb @@ -1,6 +1,3 @@ -# frozen_string_literal: true -# typed: false - require "redis" require "flipper" From 3e1a0237369dbd179e23250d141c0dccf7f504ea Mon Sep 17 00:00:00 2001 From: Cymen Vig Date: Fri, 19 Jan 2024 19:15:39 -0500 Subject: [PATCH 4/4] clean up test to be simpler/only one thread in connection pool --- spec/flipper/adapters/redis_cache_connection_pool_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/flipper/adapters/redis_cache_connection_pool_spec.rb b/spec/flipper/adapters/redis_cache_connection_pool_spec.rb index 8459ed280..e491860dd 100644 --- a/spec/flipper/adapters/redis_cache_connection_pool_spec.rb +++ b/spec/flipper/adapters/redis_cache_connection_pool_spec.rb @@ -6,8 +6,7 @@ let(:client) do options = {} options[:url] = ENV['REDIS_URL'] if ENV['REDIS_URL'] - redis = Redis.new(options) - ConnectionPool.new(size: 5, timeout: 1) { redis } + ConnectionPool.new(size: 1, timeout: 1) { Redis.new(options) } end let(:memory_adapter) do