Batch cleanup_old_experiments! into one pipelined roundtrip

Author: andrehjr

lib/split/user.rb

@@ -15,13 +15,25 @@ def initialize(context, adapter = nil)
 
     def cleanup_old_experiments!
       return if @cleaned_up
-      keys_without_finished(user.keys).each do |key|
-        experiment = Experiment.new key_without_version(key)
-        if experiment.nil? || experiment.has_winner? || experiment.start_time.nil?
-          user.delete key
-          user.delete Experiment.finished_key(key)
+      keys = keys_without_finished(user.keys)
+      names = keys.map { |key| key_without_version(key) }
+
+      unless names.empty?
+        winners, start_times = Split.redis.pipelined do |pipe|
+          pipe.hmget(:experiment_winner, *names)
+          pipe.hmget(:experiment_start_times, *names)
+        end
+
+        keys.each_with_index do |key, index|
+          has_winner = !winners[index].nil?
+          not_started = start_times[index].nil?
+          if has_winner || not_started
+            user.delete key
+            user.delete Experiment.finished_key(key)
+          end
         end
       end
+
       @cleaned_up = true
     end
 

spec/support/redis_call_counter.rb

@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+require "redis-client"
+
+module SplitRedisCallInstrumentation
+  KEY = :split_redis_call_count
+
+  def call(command, config)
+    SplitRedisCallInstrumentation.increment
+    super
+  end
+
+  def call_pipelined(commands, config)
+    SplitRedisCallInstrumentation.increment
+    super
+  end
+
+  class << self
+    def count
+      previous = Thread.current[KEY]
+      Thread.current[KEY] = 0
+      yield
+      Thread.current[KEY]
+    ensure
+      Thread.current[KEY] = previous
+    end
+
+    def increment
+      count = Thread.current[KEY]
+      Thread.current[KEY] = count + 1 if count
+    end
+  end
+end
+
+RedisClient.register(SplitRedisCallInstrumentation)
+
+RSpec::Matchers.define :make_redis_calls do |expected|
+  supports_block_expectations
+
+  match do |block|
+    @actual = SplitRedisCallInstrumentation.count(&block)
+    values_match?(expected, @actual)
+  end
+
+  failure_message do
+    "expected block to make #{description_of(expected)} Redis roundtrip(s), but made #{@actual}"
+  end
+
+  failure_message_when_negated do
+    "expected block not to make #{description_of(expected)} Redis roundtrip(s), but it did (#{@actual})"
+  end
+
+  description do
+    "make #{description_of(expected)} Redis roundtrip(s)"
+  end
+end

spec/user_spec.rb

@@ -88,6 +88,34 @@
         @subject.cleanup_old_experiments!
       end
     end
+
+    context "with many experiments" do
+      let(:user_keys) do
+        {
+          "with_winner" => "red",
+          "not_started" => "red",
+          "active" => "red"
+        }
+      end
+
+      before do
+        with_winner = Split::ExperimentCatalog.find_or_create("with_winner", "red", "blue")
+        with_winner.start
+        with_winner.winner = "red"
+
+        Split::ExperimentCatalog.find_or_create("active", "red", "blue").start
+      end
+
+      it "keeps active experiments while dropping finished/not-started ones" do
+        @subject.cleanup_old_experiments!
+
+        expect(@subject.keys).to eq(["active"])
+      end
+
+      it "batches the winner/start-time lookups into a single roundtrip" do
+        expect { @subject.cleanup_old_experiments! }.to make_redis_calls(1)
+      end
+    end
   end
 
   context "allows user to be loaded from adapter" do