diff --git a/lib/split/experiment.rb b/lib/split/experiment.rb index 3820a0cc..042a2a4a 100644 --- a/lib/split/experiment.rb +++ b/lib/split/experiment.rb @@ -102,7 +102,7 @@ def validate! end def new_record? - !@redis_storage.exists? + @redis_storage.new_record? end def ==(obj) @@ -462,6 +462,8 @@ def persist_experiment_configuration else delete_metadata end + + @redis_storage.reload end def remove_experiment_configuration @@ -469,6 +471,7 @@ def remove_experiment_configuration goals_collection.delete delete_metadata redis.del(@name) + @redis_storage.reload end def experiment_configuration_has_changed? diff --git a/lib/split/experiment_storage.rb b/lib/split/experiment_storage.rb index 96bb76fa..382ad765 100644 --- a/lib/split/experiment_storage.rb +++ b/lib/split/experiment_storage.rb @@ -13,6 +13,14 @@ def load @data ||= load! end + def reload + @data = nil + end + + def new_record? + load[:alternatives].empty? + end + def load! experiment_config = load_experiment alternatives = load_alternatives diff --git a/lib/split/user.rb b/lib/split/user.rb index 7d9b90c1..b7b32c73 100644 --- a/lib/split/user.rb +++ b/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 @@ -43,10 +55,17 @@ def cleanup_old_versions!(experiment) end def active_experiments + experiments_by_key = keys_without_finished(user.keys).each_with_object({}) do |key, memo| + memo[key] = Metric.possible_experiments(key_without_version(key)) + end + + names = experiments_by_key.values.flatten.map(&:name).uniq + winners = names.empty? ? {} : names.zip(Split.redis.hmget(:experiment_winner, *names)).to_h + experiment_pairs = {} - keys_without_finished(user.keys).each do |key| - Metric.possible_experiments(key_without_version(key)).each do |experiment| - if !experiment.has_winner? + experiments_by_key.each do |key, experiments| + experiments.each do |experiment| + unless winners[experiment.name] experiment_pairs[key_without_version(key)] = user[key] end end diff --git a/spec/support/redis_call_counter.rb b/spec/support/redis_call_counter.rb new file mode 100644 index 00000000..fc43dfdf --- /dev/null +++ b/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 diff --git a/spec/user_spec.rb b/spec/user_spec.rb index 23f5e5c6..5e46210f 100644 --- a/spec/user_spec.rb +++ b/spec/user_spec.rb @@ -88,6 +88,55 @@ @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 "#active_experiments" do + let(:user_keys) { { "with_winner" => "red", "active" => "red" } } + + 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 "excludes experiments that already have a winner" do + expect(@subject.active_experiments).to eq("active" => "red") + end + + it "fetches every experiment's winner in a single call" do + expect(Split.redis).to receive(:hmget).with(:experiment_winner, any_args).once.and_call_original + @subject.active_experiments + end end context "allows user to be loaded from adapter" do