Skip to content

Commit bca684d

Browse files
committed
Remove Split::Cache and make config.cache a no-op
The cache lives in each process, so a winner set from the dashboard stayed stale in the other workers. It only saves 10 of 64 roundtrips on a dashboard page and none on ab_test, so it is no longer worth the risk. config.cache stays as an accessor so apps that set it keep booting.
1 parent fa76728 commit bca684d

7 files changed

Lines changed: 24 additions & 149 deletions

File tree

‎README.md‎

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -779,22 +779,11 @@ split_config = YAML.load_file(Rails.root.join('config', 'split.yml'))
779779
Split.redis = split_config[Rails.env]
780780
```
781781

782-
### Redis Caching (v4.0+)
782+
### Redis Caching
783783

784-
> [!WARNING]
785-
> The caching mechanism has known issues in multi-process environments (e.g., Puma) and multi-server deployments with load balancers (typical production environments). Cache invalidation does not propagate across processes or servers, which can lead to inconsistent experiment data being served to users. See [#735](https://github.com/splitrb/split/issues/735) for details. Use with caution in production environments.
786-
787-
In some high-volume usage scenarios, Redis load can be incurred by repeated
788-
fetches for fairly static data. Enabling caching will reduce this load.
789-
790-
```ruby
791-
Split.configuration.cache = true
792-
````
793-
794-
This currently caches:
795-
- `Split::ExperimentCatalog.find`
796-
- `Split::Experiment.start_time`
797-
- `Split::Experiment.winner`
784+
`Split.configuration.cache` no longer has any effect and can be removed. The cache
785+
did not stay consistent across processes (see [#735](https://github.com/splitrb/split/issues/735)),
786+
and Split now makes few enough Redis calls that it is no longer needed.
798787

799788
## Namespaces
800789

‎lib/split.rb‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
require "split/algorithms/weighted_sample"
88
require "split/algorithms/whiplash"
99
require "split/alternative"
10-
require "split/cache"
1110
require "split/configuration"
1211
require "split/encapsulated_helper"
1312
require "split/exceptions"
@@ -69,10 +68,6 @@ def configure
6968
self.configuration ||= Configuration.new
7069
yield(configuration)
7170
end
72-
73-
def cache(namespace, key, &block)
74-
Split::Cache.fetch(namespace, key, &block)
75-
end
7671
end
7772

7873
# Check to see if being run in a Rails application. If so, wait until before_initialize to run configuration so Gems that create ENV variables have the chance to initialize first.

‎lib/split/cache.rb‎

Lines changed: 0 additions & 27 deletions
This file was deleted.

‎lib/split/experiment.rb‎

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,9 @@ class Experiment
1717
}
1818

1919
def self.find(name)
20-
Split.cache(:experiments, name) do
21-
experiment = Experiment.new(name)
22-
experiment.load_from_redis
23-
experiment unless experiment.alternatives.empty?
24-
end
20+
experiment = Experiment.new(name)
21+
experiment.load_from_redis
22+
experiment unless experiment.alternatives.empty?
2523
end
2624

2725
def self.find_many(names)
@@ -149,13 +147,11 @@ def alternatives=(alts)
149147
end
150148

151149
def winner
152-
Split.cache(:experiment_winner, name) do
153-
experiment_winner = redis.hget(:experiment_winner, name)
154-
if experiment_winner
155-
Split::Alternative.new(experiment_winner, name)
156-
else
157-
nil
158-
end
150+
experiment_winner = redis.hget(:experiment_winner, name)
151+
if experiment_winner
152+
Split::Alternative.new(experiment_winner, name)
153+
else
154+
nil
159155
end
160156
end
161157

@@ -181,7 +177,6 @@ def control
181177
def reset_winner
182178
redis.hdel(:experiment_winner, name)
183179
@has_winner = false
184-
Split::Cache.clear_key(@name)
185180
end
186181

187182
def start
@@ -191,9 +186,7 @@ def start
191186
def start_time
192187
return @start_time if defined?(@start_time)
193188

194-
@start_time = Split.cache(:experiment_start_times, @name) do
195-
parse_start_time(redis.hget(:experiment_start_times, @name))
196-
end
189+
@start_time = parse_start_time(redis.hget(:experiment_start_times, @name))
197190
end
198191

199192
def prefetch_trial_state!
@@ -258,7 +251,6 @@ def resettable?
258251

259252
def reset
260253
Split.configuration.on_before_experiment_reset.call(self)
261-
Split::Cache.clear_key(@name)
262254
alternatives.each(&:reset)
263255
reset_winner
264256
Split.configuration.on_experiment_reset.call(self)

‎spec/cache_spec.rb‎

Lines changed: 0 additions & 84 deletions
This file was deleted.

‎spec/experiment_spec.rb‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,17 @@ def alternative(color)
247247
it "should have no winner initially" do
248248
expect(experiment.winner).to be_nil
249249
end
250+
251+
it "should see a winner changed by another process when the cache setting is on" do
252+
Split.configuration.cache = true
253+
experiment.save
254+
experiment.winner = "red"
255+
expect(experiment.winner.name).to eq("red")
256+
257+
Split.redis.hset(:experiment_winner, "link_color", "blue")
258+
259+
expect(experiment.winner.name).to eq("blue")
260+
end
250261
end
251262

252263
describe "winner=" do

‎spec/spec_helper.rb‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ module GlobalSharedContext
2323
Split.redis = Redis.new
2424
Split.redis.select(10)
2525
Split.redis.flushdb
26-
Split::Cache.clear
2726
@ab_user = mock_user
2827
@params = nil
2928
end

0 commit comments

Comments
 (0)