From d0c26297b866aa288c91ba52351b3562021f3169 Mon Sep 17 00:00:00 2001
From: Sylvester Chin <schin@gitlab.com>
Date: Thu, 22 Feb 2024 13:19:27 +0800
Subject: [PATCH] Use multistore pool for RepositoryCache.cache_store
---
lib/gitlab/redis/repository_cache.rb | 2 +-
lib/gitlab/repository_cache.rb | 38 ++++++-
.../lib/gitlab/redis/repository_cache_spec.rb | 4 +
spec/lib/gitlab/repository_cache_spec.rb | 98 ++++++++++++++++++-
4 files changed, 133 insertions(+), 9 deletions(-)
diff --git a/lib/gitlab/redis/repository_cache.rb b/lib/gitlab/redis/repository_cache.rb
index 931c87bcd84dc..4d8d19ca2c085 100644
--- a/lib/gitlab/redis/repository_cache.rb
+++ b/lib/gitlab/redis/repository_cache.rb
@@ -14,7 +14,7 @@ def config_fallback
def cache_store
@cache_store ||= RepositoryCacheStore.new(
- redis: pool,
+ redis: multistore_pool,
pool: false,
compress: Gitlab::Utils.to_boolean(ENV.fetch('ENABLE_REDIS_CACHE_COMPRESSION', '1')),
namespace: Cache::CACHE_NAMESPACE,
diff --git a/lib/gitlab/repository_cache.rb b/lib/gitlab/repository_cache.rb
index 498eaf92381d6..89648705ae27e 100644
--- a/lib/gitlab/repository_cache.rb
+++ b/lib/gitlab/repository_cache.rb
@@ -18,23 +18,33 @@ def cache_key(type)
end
def expire(key)
- backend.delete(cache_key(key))
+ borrow_multistore_connection do
+ backend.delete(cache_key(key))
+ end
end
def fetch(key, &block)
- backend.fetch(cache_key(key), &block)
+ borrow_multistore_connection do
+ backend.fetch(cache_key(key), &block)
+ end
end
def exist?(key)
- backend.exist?(cache_key(key))
+ borrow_multistore_connection do
+ backend.exist?(cache_key(key))
+ end
end
def read(key)
- backend.read(cache_key(key))
+ borrow_multistore_connection do
+ backend.read(cache_key(key))
+ end
end
def write(key, value, *args)
- backend.write(cache_key(key), value, *args)
+ borrow_multistore_connection do
+ backend.write(cache_key(key), value, *args)
+ end
end
def fetch_without_caching_false(key, &block)
@@ -49,6 +59,24 @@ def fetch_without_caching_false(key, &block)
value
end
+ def borrow_multistore_connection
+ if Feature.disabled?(:use_primary_store_as_default_for_repository_cache, type: :gitlab_com_derisk) &&
+ Feature.disabled?(:use_primary_and_secondary_stores_for_repository_cache, type: :gitlab_com_derisk)
+ return yield
+ end
+
+ # TODO: to be removed after repository cache migration alongside Gitlab::Redis::ClusterRepositoryCache
+ # See https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/2854
+ #
+ # Borrows connections for multistore before letting Rails RedisCacheStore perform redis operations
+ # RedisCacheStore performs another `.with` but does not check out an extra connection since it re-uses the
+ # previously checked out connection on the same thread.
+ # See https://github.com/mperham/connection_pool/blob/v2.4.1/lib/connection_pool.rb#L122
+ Gitlab::Redis::RepositoryCache.with do |_c|
+ yield
+ end
+ end
+
def self.store
Gitlab::Redis::RepositoryCache.cache_store
end
diff --git a/spec/lib/gitlab/redis/repository_cache_spec.rb b/spec/lib/gitlab/redis/repository_cache_spec.rb
index 8580853fd8563..0f5d816b56270 100644
--- a/spec/lib/gitlab/redis/repository_cache_spec.rb
+++ b/spec/lib/gitlab/redis/repository_cache_spec.rb
@@ -10,6 +10,10 @@
it 'has a default ttl of 8 hours' do
expect(described_class.cache_store.options[:expires_in]).to eq(8.hours)
end
+
+ it 'uses a pool of multistore connections' do
+ expect(described_class.cache_store.redis.checkout).to be_an_instance_of(Gitlab::Redis::MultiStore)
+ end
end
it 'migrates from self to ClusterRepositoryCache' do
diff --git a/spec/lib/gitlab/repository_cache_spec.rb b/spec/lib/gitlab/repository_cache_spec.rb
index 3277135246d74..1a982f2248786 100644
--- a/spec/lib/gitlab/repository_cache_spec.rb
+++ b/spec/lib/gitlab/repository_cache_spec.rb
@@ -56,19 +56,85 @@
end
end
+ shared_examples 'borrows connections for multistore' do
+ before do
+ if Gitlab::Redis::RepositoryCache.instance_variable_defined?(:@multistore_pool)
+ Gitlab::Redis::RepositoryCache.remove_instance_variable(:@multistore_pool)
+ end
+ end
+
+ it 'borrows connections for the multistore' do
+ expect_next_instance_of(Gitlab::Redis::MultiStore) do |m|
+ expect(m).to receive(:with_borrowed_connection).and_call_original
+ end
+
+ operation
+ end
+ end
+
+ shared_examples 'compatiblity with multistore' do
+ it_behaves_like 'borrows connections for multistore'
+
+ context 'with use_primary_and_secondary_stores_for_repository_cache disabled' do
+ before do
+ stub_feature_flags(
+ use_primary_store_as_default_for_repository_cache: true,
+ use_primary_and_secondary_stores_for_repository_cache: false
+ )
+ end
+
+ it_behaves_like 'borrows connections for multistore'
+ end
+
+ context 'with use_primary_store_as_default_for_repository_cache disabled' do
+ before do
+ stub_feature_flags(
+ use_primary_store_as_default_for_repository_cache: false,
+ use_primary_and_secondary_stores_for_repository_cache: true
+ )
+ end
+
+ it_behaves_like 'borrows connections for multistore'
+ end
+
+ context 'with both feature flags disabled' do
+ before do
+ stub_feature_flags(
+ use_primary_store_as_default_for_repository_cache: false,
+ use_primary_and_secondary_stores_for_repository_cache: false
+ )
+ end
+
+ it 'does not borrow connections for the multistore' do
+ expect(Gitlab::Redis::RepositoryCache).not_to receive(:with)
+
+ operation
+ end
+ end
+ end
+
describe '#expire' do
+ subject(:operation) { cache.expire(:foo) }
+
it 'expires the given key from the cache' do
- cache.expire(:foo)
+ operation
+
expect(backend).to have_received(:delete).with("foo:#{namespace}")
end
+
+ it_behaves_like 'compatiblity with multistore'
end
describe '#fetch' do
+ subject(:operation) { cache.fetch(:bar) }
+
it 'fetches the given key from the cache' do
- cache.fetch(:bar)
+ operation
expect(backend).to have_received(:fetch).with("bar:#{namespace}")
end
+ it_behaves_like 'compatiblity with multistore'
+
it 'accepts a block' do
p = -> {}
@@ -78,11 +144,15 @@
end
describe '#write' do
+ subject(:operation) { cache.write(:test, 'test') }
+
it 'writes the given key and value to the cache' do
- cache.write(:test, 'test')
+ operation
expect(backend).to have_received(:write).with("test:#{namespace}", 'test')
end
+ it_behaves_like 'compatiblity with multistore'
+
it 'passes additional options to the backend' do
cache.write(:test, 'test', expires_in: 10.minutes)
expect(backend).to have_received(:write).with("test:#{namespace}", 'test', expires_in: 10.minutes)
@@ -173,4 +243,26 @@
end
end
end
+
+ describe '#exist?' do
+ subject(:operation) { cache.exist?(:test) }
+
+ it 'calls exists? on backend' do
+ operation
+ expect(backend).to have_received(:exist?).with("test:#{namespace}")
+ end
+
+ it_behaves_like 'compatiblity with multistore'
+ end
+
+ describe '#read' do
+ subject(:operation) { cache.read(:test) }
+
+ it 'calls read on backend' do
+ operation
+ expect(backend).to have_received(:read).with("test:#{namespace}")
+ end
+
+ it_behaves_like 'compatiblity with multistore'
+ end
end
--
GitLab