Cache get_all separately

Doing a cache write for hundreds of uncached features doesn't work so well. Wish there was a bulk write.

I could use write_multi but only redis cache seems to support that.

Author: jnunemaker

lib/flipper/adapters/active_support_cache_store.rb

@@ -41,6 +41,7 @@ def enable(feature, gate, thing)
         if @write_through
           result = @adapter.enable(feature, gate, thing)
           cache_write feature_cache_key(feature.key), @adapter.get(feature)
+          expire_get_all_cache
           result
         else
           super
@@ -51,6 +52,7 @@ def disable(feature, gate, thing)
         if @write_through
           result = @adapter.disable(feature, gate, thing)
           cache_write feature_cache_key(feature.key), @adapter.get(feature)
+          expire_get_all_cache
           result
         else
           super

lib/flipper/adapters/cache_base.rb

@@ -17,6 +17,9 @@ class CacheBase
       # Public: The cache key where the set of known features is cached.
       attr_reader :features_cache_key
 
+      # Public: The cache key where the set of all features with gates is cached.
+      attr_reader :get_all_cache_key
+
       # Public: Alias expires_in to ttl for compatibility.
       alias_method :expires_in, :ttl
 
@@ -29,16 +32,24 @@ def initialize(adapter, cache, ttl = 300, prefix: nil)
         @namespace = "flipper/#{@cache_version}"
         @namespace = @namespace.prepend(prefix) if prefix
         @features_cache_key = "#{@namespace}/features"
+        @get_all_cache_key = "#{@namespace}/get_all"
+      end
+
+      # Public: Expire the cache for the set of all features with gates.
+      def expire_get_all_cache
+        cache_delete @get_all_cache_key
       end
 
       # Public: Expire the cache for the set of known feature names.
       def expire_features_cache
         cache_delete @features_cache_key
+        expire_get_all_cache
       end
 
       # Public: Expire the cache for a given feature.
       def expire_feature_cache(key)
         cache_delete feature_cache_key(key)
+        expire_get_all_cache
       end
 
       # Public
@@ -80,8 +91,11 @@ def get_multi(features)
 
       # Public
       def get_all(**kwargs)
-        features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) }
-        read_many_features(features)
+        cache_fetch(@get_all_cache_key) {
+          result = read_all_features(**kwargs)
+          cache_write @features_cache_key, result.keys.to_set
+          result
+        }
       end
 
       # Public
@@ -107,6 +121,10 @@ def feature_cache_key(key)
 
       private
 
+      def read_all_features(**kwargs)
+        @adapter.get_all(**kwargs)
+      end
+
       # Private: Returns the Set of known feature keys.
       def read_feature_keys
         cache_fetch(@features_cache_key) { @adapter.features }

spec/flipper/adapters/active_support_cache_store_spec.rb

@@ -61,19 +61,27 @@
     adapter.features
     expect(cache.read("flipper/v1/features")).not_to be(nil)
 
+    adapter.get_all
+    expect(cache.read("flipper/v1/get_all")).not_to be(nil)
+
     # expire cache
     adapter.expire_features_cache
     expect(cache.read("flipper/v1/features")).to be(nil)
+    expect(cache.read("flipper/v1/get_all")).to be(nil)
   end
 
   it "can expire feature cache" do
     # cache the features
     adapter.get(flipper[:stats])
     expect(cache.read("flipper/v1/feature/stats")).not_to be(nil)
 
+    adapter.get_all
+    expect(cache.read("flipper/v1/get_all")).not_to be(nil)
+
     # expire cache
     adapter.expire_feature_cache("stats")
     expect(cache.read("flipper/v1/feature/stats")).to be(nil)
+    expect(cache.read("flipper/v1/get_all")).to be(nil)
   end
 
   it "can generate feature cache key" do
@@ -88,6 +96,10 @@
       expect(adapter.features_cache_key).to eq("foo/flipper/v1/features")
     end
 
+    it "knows get_all_cache_key" do
+      expect(adapter.get_all_cache_key).to eq("foo/flipper/v1/get_all")
+    end
+
     it "can generate feature cache key" do
       expect(adapter.feature_cache_key("stats")).to eq("foo/flipper/v1/feature/stats")
     end
@@ -109,9 +121,10 @@
       adapter.get_all
 
       # verify cached with prefix
+      get_all_cache_value = cache.read("foo/flipper/v1/get_all")
       expect(cache.read("foo/flipper/v1/features")).to eq(Set["stats", "search"])
-      expect(cache.read("foo/flipper/v1/feature/search")[:percentage_of_actors]).to eq("10")
-      expect(cache.read("foo/flipper/v1/feature/stats")[:boolean]).to eq("true")
+      expect(get_all_cache_value["search"][:percentage_of_actors]).to eq("10")
+      expect(get_all_cache_value["stats"][:boolean]).to eq("true")
     end
   end
 
@@ -120,11 +133,13 @@
 
     before do
       adapter.get(feature)
+      adapter.get_all
       adapter.remove(feature)
     end
 
     it 'expires feature and deletes the cache' do
       expect(cache.read("flipper/v1/feature/#{feature.key}")).to be_nil
+      expect(cache.read("flipper/v1/get_all")).to be(nil)
       expect(cache.exist?("flipper/v1/feature/#{feature.key}")).to be(false)
       expect(feature).not_to be_enabled
     end
@@ -144,10 +159,13 @@
     let(:feature) { flipper[:stats] }
 
     before do
+      adapter.get(feature)
+      adapter.get_all
       adapter.enable(feature, feature.gate(:boolean), Flipper::Types::Boolean.new(true))
     end
 
     it 'enables feature and deletes the cache' do
+      expect(cache.read("flipper/v1/get_all")).to be(nil)
       expect(cache.read("flipper/v1/feature/#{feature.key}")).to be_nil
       expect(cache.exist?("flipper/v1/feature/#{feature.key}")).to be(false)
       expect(feature).to be_enabled
@@ -157,6 +175,7 @@
       let(:write_through) { true }
 
       it 'expires feature and writes to the cache' do
+        expect(cache.read("flipper/v1/get_all")).to be(nil)
         expect(cache.exist?("flipper/v1/feature/#{feature.key}")).to be(true)
         expect(cache.read("flipper/v1/feature/#{feature.key}")).to include(boolean: 'true')
         expect(feature).to be_enabled
@@ -168,10 +187,13 @@
     let(:feature) { flipper[:stats] }
 
     before do
+      adapter.get(feature)
+      adapter.get_all
       adapter.disable(feature, feature.gate(:boolean), Flipper::Types::Boolean.new)
     end
 
     it 'disables feature and deletes the cache' do
+      expect(cache.read("flipper/v1/get_all")).to be(nil)
       expect(cache.read("flipper/v1/feature/#{feature.key}")).to be_nil
       expect(cache.exist?("flipper/v1/feature/#{feature.key}")).to be(false)
       expect(feature).not_to be_enabled
@@ -224,8 +246,10 @@
 
     it 'warms all features' do
       adapter.get_all
-      expect(cache.read("flipper/v1/feature/#{stats.key}")[:boolean]).to eq('true')
-      expect(cache.read("flipper/v1/feature/#{search.key}")[:boolean]).to be(nil)
+      get_all_cache_value = cache.read("flipper/v1/get_all")
+      expect(get_all_cache_value).not_to be(nil)
+      expect(get_all_cache_value["stats"][:boolean]).to eq('true')
+      expect(get_all_cache_value["search"][:boolean]).to be(nil)
       expect(cache.read("flipper/v1/features")).to eq(Set["stats", "search"])
     end
 
@@ -236,9 +260,8 @@
     it 'only invokes two calls to wrapped adapter (for features set and gate data for each feature in set)' do
       memory_adapter.reset
       5.times { adapter.get_all }
-      expect(memory_adapter.count(:features)).to eq(1)
-      expect(memory_adapter.count(:get_multi)).to eq(1)
-      expect(memory_adapter.count).to eq(2)
+      expect(memory_adapter.count(:get_all)).to eq(1)
+      expect(memory_adapter.count).to eq(1)
     end
   end
 

spec/flipper/adapters/dalli_spec.rb

@@ -79,9 +79,11 @@
       flipper.enable(:stats)
 
       # populate the cache
-      adapter.get_all
+      adapter.features
+      adapter.get_multi([flipper[:stats], flipper[:search]])
 
       # verify cached with prefix
+      expect(cache.get("foo/flipper/v1/features")).to eq(Set["stats", "search"])
       expect(cache.get("foo/flipper/v1/feature/search")[:percentage_of_actors]).to eq("10")
       expect(cache.get("foo/flipper/v1/feature/stats")[:boolean]).to eq("true")
     end
@@ -132,21 +134,21 @@
 
     it 'warms all features' do
       adapter.get_all
-      expect(cache.get("flipper/v1/feature/#{stats.key}")[:boolean]).to eq('true')
-      expect(cache.get("flipper/v1/feature/#{search.key}")[:boolean]).to be(nil)
+      get_all_cache_value = cache.get("flipper/v1/get_all")
+      expect(get_all_cache_value["stats"][:boolean]).to eq("true")
+      expect(get_all_cache_value["search"][:boolean]).to be(nil)
       expect(cache.get("flipper/v1/features")).to eq(Set["stats", "search"])
     end
 
     it 'returns same result when already cached' do
       expect(adapter.get_all).to eq(adapter.get_all)
     end
 
-    it 'only invokes two calls to wrapped adapter (for features set and gate data for each feature in set)' do
+    it 'only invokes one call to wrapped adapter (for features set and gate data for each feature in set)' do
       memory_adapter.reset
       5.times { adapter.get_all }
-      expect(memory_adapter.count(:features)).to eq(1)
-      expect(memory_adapter.count(:get_multi)).to eq(1)
-      expect(memory_adapter.count).to eq(2)
+      expect(memory_adapter.count(:get_all)).to eq(1)
+      expect(memory_adapter.count).to eq(1)
     end
   end
 

spec/flipper/adapters/redis_cache_spec.rb

@@ -80,7 +80,8 @@
       flipper.enable(:stats)
 
       # populate the cache
-      adapter.get_all
+      adapter.features
+      adapter.get_multi([flipper[:stats], flipper[:search]])
 
       # verify cached with prefix
       expect(Marshal.load(client.get("foo/flipper/v1/features"))).to eq(Set["stats", "search"])
@@ -145,21 +146,20 @@
 
     it 'warms all features' do
       adapter.get_all
-      expect(Marshal.load(client.get("flipper/v1/feature/#{stats.key}"))[:boolean]).to eq('true')
-      expect(Marshal.load(client.get("flipper/v1/feature/#{search.key}"))[:boolean]).to be(nil)
+      get_all_cache_value = client.get("flipper/v1/get_all")
+      expect(Marshal.load(get_all_cache_value)["stats"][:boolean]).to eq("true")
       expect(Marshal.load(client.get("flipper/v1/features"))).to eq(Set["stats", "search"])
     end
 
     it 'returns same result when already cached' do
       expect(adapter.get_all).to eq(adapter.get_all)
     end
 
-    it 'only invokes two calls to wrapped adapter (for features set and gate data for each feature in set)' do
+    it 'only invokes one calls to wrapped adapter (for features set and gate data for each feature in set)' do
       memory_adapter.reset
       5.times { adapter.get_all }
-      expect(memory_adapter.count(:features)).to eq(1)
-      expect(memory_adapter.count(:get_multi)).to eq(1)
-      expect(memory_adapter.count).to eq(2)
+      expect(memory_adapter.count(:get_all)).to eq(1)
+      expect(memory_adapter.count).to eq(1)
     end
   end
 

spec/flipper/middleware/memoizer_spec.rb

@@ -470,18 +470,15 @@ def get(uri, params = {}, env = {}, &block)
 
       get '/', {}, 'flipper' => flipper
       expect(logged_cached.count(:get_all)).to be(1)
-      expect(logged_memory.count(:features)).to be(1)
-      expect(logged_memory.count(:get_multi)).to be(1)
+      expect(logged_memory.count(:get_all)).to be(1)
 
       get '/', {}, 'flipper' => flipper
       expect(logged_cached.count(:get_all)).to be(2)
-      expect(logged_memory.count(:features)).to be(1)
-      expect(logged_memory.count(:get_multi)).to be(1)
+      expect(logged_memory.count(:get_all)).to be(1)
 
       get '/', {}, 'flipper' => flipper
       expect(logged_cached.count(:get_all)).to be(3)
-      expect(logged_memory.count(:features)).to be(1)
-      expect(logged_memory.count(:get_multi)).to be(1)
+      expect(logged_memory.count(:get_all)).to be(1)
     end
   end
 end