Skip to content
Snippets Groups Projects
Commit 3c2718ca authored by Sylvester Chin's avatar Sylvester Chin :red_circle:
Browse files

Remove use_pipeline_over_multikey feature flag

This MR removes the use of the feature flag for benchmarking Redis
pipelines in GitLab Rails.

Changelog: other
MR: !118884
parent e979c2f4
No related branches found
No related tags found
2 merge requests!123111Remove use_pipeline_over_multikey feature flag,!119439Draft: Prevent file variable content expansion in downstream pipeline
---
name: use_pipeline_over_multikey
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118884
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/409436
milestone: '16.0'
type: development
group: group::scalability
default_enabled: false
......@@ -65,7 +65,7 @@ def delete_by_email(*emails)
keys = emails.map { |email| email_key(email) }
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
if ::Feature.enabled?(:use_pipeline_over_multikey) || Gitlab::Redis::ClusterUtil.cluster?(redis)
if Gitlab::Redis::ClusterUtil.cluster?(redis)
Gitlab::Redis::ClusterUtil.batch_unlink(keys, redis)
else
redis.unlink(*keys)
......
......@@ -41,7 +41,7 @@ def read_multiple(raw_keys)
content =
with_redis do |redis|
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
if ::Feature.enabled?(:use_pipeline_over_multikey) || Gitlab::Redis::ClusterUtil.cluster?(redis)
if Gitlab::Redis::ClusterUtil.cluster?(redis)
Gitlab::Redis::CrossSlot::Pipeline.new(redis).pipelined do |pipeline|
keys.each { |key| pipeline.get(key) }
end
......@@ -72,7 +72,7 @@ def clear_multiple(raw_keys)
with_redis do |redis|
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
if ::Feature.enabled?(:use_pipeline_over_multikey) || Gitlab::Redis::ClusterUtil.cluster?(redis)
if Gitlab::Redis::ClusterUtil.cluster?(redis)
Gitlab::Redis::ClusterUtil.batch_unlink(keys, redis)
else
redis.del(keys)
......
......@@ -11,13 +11,12 @@ def initialize(expires_in: 10.minutes)
end
def clear_cache!(key)
use_pipeline = ::Feature.enabled?(:use_pipeline_over_multikey)
with do |redis|
keys = read(key).map { |value| "#{cache_namespace}:#{value}" }
keys << cache_key(key)
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
if use_pipeline || Gitlab::Redis::ClusterUtil.cluster?(redis)
if Gitlab::Redis::ClusterUtil.cluster?(redis)
Gitlab::Redis::ClusterUtil.batch_unlink(keys, redis)
else
redis.pipelined do |pipeline|
......
......@@ -22,7 +22,7 @@ def expire(*keys)
keys_to_expire = keys.map { |key| cache_key(key) }
Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
if ::Feature.enabled?(:use_pipeline_over_multikey) || Gitlab::Redis::ClusterUtil.cluster?(redis)
if Gitlab::Redis::ClusterUtil.cluster?(redis)
Gitlab::Redis::ClusterUtil.batch_unlink(keys_to_expire, redis)
else
redis.unlink(*keys_to_expire)
......
......@@ -62,62 +62,54 @@ def read(key, subkey)
end
describe "#delete_by_email" do
shared_examples 'delete emails' do
subject { described_class.delete_by_email(*emails) }
subject { described_class.delete_by_email(*emails) }
before do
perform_fetch
end
before do
perform_fetch
end
context "no emails, somehow" do
let(:emails) { [] }
context "no emails, somehow" do
let(:emails) { [] }
it { is_expected.to eq(0) }
end
it { is_expected.to eq(0) }
end
context "single email" do
let(:emails) { "foo@bar.com" }
context "single email" do
let(:emails) { "foo@bar.com" }
it "removes the email" do
expect(read(key, "20:2:true")).to eq(avatar_path)
it "removes the email" do
expect(read(key, "20:2:true")).to eq(avatar_path)
expect(subject).to eq(1)
expect(subject).to eq(1)
expect(read(key, "20:2:true")).to eq(nil)
end
expect(read(key, "20:2:true")).to eq(nil)
end
end
context "multiple emails" do
let(:emails) { ["foo@bar.com", "missing@baz.com"] }
context "multiple emails" do
let(:emails) { ["foo@bar.com", "missing@baz.com"] }
it "removes the emails it finds" do
expect(read(key, "20:2:true")).to eq(avatar_path)
it "removes the emails it finds" do
expect(read(key, "20:2:true")).to eq(avatar_path)
expect(subject).to eq(1)
expect(subject).to eq(1)
expect(read(key, "20:2:true")).to eq(nil)
end
expect(read(key, "20:2:true")).to eq(nil)
end
end
context 'when deleting over 1000 emails' do
it 'deletes in batches of 1000' do
Gitlab::Redis::Cache.with do |redis|
expect(redis).to receive(:pipelined).at_least(2).and_call_original
if Gitlab::Redis::ClusterUtil.cluster?(redis)
expect(redis).to receive(:pipelined).at_least(2).and_call_original
else
expect(redis).to receive(:unlink).and_call_original
end
end
described_class.delete_by_email(*(Array.new(1001) { |i| i }))
end
end
context 'when feature flag disabled' do
before do
stub_feature_flags(use_pipeline_over_multikey: false)
end
it_behaves_like 'delete emails'
end
it_behaves_like 'delete emails'
end
end
......@@ -41,81 +41,57 @@ def fake_file(offset)
end
describe '#read_multiple' do
shared_examples 'read multiple keys' do
it 'reads multiple keys and serializes content into Gitlab::Diff::Line objects' do
described_class.write_multiple(mapping)
found = described_class.read_multiple(mapping.keys)
expect(found.size).to eq(2)
expect(found.first.size).to eq(2)
expect(found.first).to all(be_a(Gitlab::Diff::Line))
end
it 'returns nil when cached key is not found' do
described_class.write_multiple(mapping)
it 'reads multiple keys and serializes content into Gitlab::Diff::Line objects' do
described_class.write_multiple(mapping)
found = described_class.read_multiple([2, 3])
found = described_class.read_multiple(mapping.keys)
expect(found.size).to eq(2)
expect(found.size).to eq(2)
expect(found.first.size).to eq(2)
expect(found.first).to all(be_a(Gitlab::Diff::Line))
end
expect(found.first).to eq(nil)
expect(found.second.size).to eq(2)
expect(found.second).to all(be_a(Gitlab::Diff::Line))
end
it 'returns nil when cached key is not found' do
described_class.write_multiple(mapping)
it 'returns lines which rich_text are HTML-safe' do
described_class.write_multiple(mapping)
found = described_class.read_multiple([2, 3])
found = described_class.read_multiple(mapping.keys)
rich_texts = found.flatten.map(&:rich_text)
expect(found.size).to eq(2)
expect(rich_texts).to all(be_html_safe)
end
expect(found.first).to eq(nil)
expect(found.second.size).to eq(2)
expect(found.second).to all(be_a(Gitlab::Diff::Line))
end
context 'when feature flag is disabled' do
before do
stub_feature_flags(use_pipeline_over_multikey: false)
end
it 'returns lines which rich_text are HTML-safe' do
described_class.write_multiple(mapping)
it_behaves_like 'read multiple keys'
end
found = described_class.read_multiple(mapping.keys)
rich_texts = found.flatten.map(&:rich_text)
it_behaves_like 'read multiple keys'
expect(rich_texts).to all(be_html_safe)
end
end
describe '#clear_multiple' do
shared_examples 'delete multiple keys' do
it 'removes all named keys' do
described_class.write_multiple(mapping)
described_class.clear_multiple(mapping.keys)
expect(described_class.read_multiple(mapping.keys)).to all(be_nil)
end
it 'removes all named keys' do
described_class.write_multiple(mapping)
it 'only removed named keys' do
to_clear, to_leave = mapping.keys
described_class.clear_multiple(mapping.keys)
described_class.write_multiple(mapping)
described_class.clear_multiple([to_clear])
expect(described_class.read_multiple(mapping.keys)).to all(be_nil)
end
cleared, left = described_class.read_multiple([to_clear, to_leave])
it 'only removed named keys' do
to_clear, to_leave = mapping.keys
expect(cleared).to be_nil
expect(left).to all(be_a(Gitlab::Diff::Line))
end
end
described_class.write_multiple(mapping)
described_class.clear_multiple([to_clear])
context 'when feature flag is disabled' do
before do
stub_feature_flags(use_pipeline_over_multikey: false)
end
cleared, left = described_class.read_multiple([to_clear, to_leave])
it_behaves_like 'delete multiple keys'
expect(cleared).to be_nil
expect(left).to all(be_a(Gitlab::Diff::Line))
end
it_behaves_like 'delete multiple keys'
end
end
......@@ -46,26 +46,16 @@
end
describe '#clear_cache!', :use_clean_rails_redis_caching do
shared_examples 'clears cache' do
it 'deletes the cached items' do
# Cached key and value
Rails.cache.write('test_item', 'test_value')
# Add key to set
cache.write(cache_prefix, 'test_item')
expect(cache.read(cache_prefix)).to contain_exactly('test_item')
cache.clear_cache!(cache_prefix)
expect(cache.read(cache_prefix)).to be_empty
end
end
it 'deletes the cached items' do
# Cached key and value
Rails.cache.write('test_item', 'test_value')
# Add key to set
cache.write(cache_prefix, 'test_item')
context 'when featuer flag disabled' do
before do
stub_feature_flags(use_pipeline_over_multikey: false)
end
expect(cache.read(cache_prefix)).to contain_exactly('test_item')
cache.clear_cache!(cache_prefix)
it_behaves_like 'clears cache'
expect(cache.read(cache_prefix)).to be_empty
end
context 'when key size is large' do
......@@ -75,14 +65,16 @@
it 'sends multiple pipelines of 1000 unlinks' do
Gitlab::Redis::Cache.with do |redis|
expect(redis).to receive(:pipelined).at_least(2).and_call_original
if Gitlab::Redis::ClusterUtil.cluster?(redis)
expect(redis).to receive(:pipelined).at_least(2).and_call_original
else
expect(redis).to receive(:pipelined).once.and_call_original
end
end
cache.clear_cache!(cache_prefix)
end
end
it_behaves_like 'clears cache'
end
describe '#include?' do
......
......@@ -77,70 +77,64 @@
end
describe '#expire' do
shared_examples 'expires varying amount of keys' do
subject { cache.expire(*keys) }
subject { cache.expire(*keys) }
before do
cache.write(:foo, ['value'])
cache.write(:bar, ['value2'])
end
before do
cache.write(:foo, ['value'])
cache.write(:bar, ['value2'])
end
it 'actually wrote the values' do
expect(cache.read(:foo)).to contain_exactly('value')
expect(cache.read(:bar)).to contain_exactly('value2')
end
it 'actually wrote the values' do
expect(cache.read(:foo)).to contain_exactly('value')
expect(cache.read(:bar)).to contain_exactly('value2')
end
context 'single key' do
let(:keys) { %w(foo) }
context 'single key' do
let(:keys) { %w(foo) }
it { is_expected.to eq(1) }
it { is_expected.to eq(1) }
it 'deletes the given key from the cache' do
subject
it 'deletes the given key from the cache' do
subject
expect(cache.read(:foo)).to be_empty
end
expect(cache.read(:foo)).to be_empty
end
end
context 'multiple keys' do
let(:keys) { %w(foo bar) }
context 'multiple keys' do
let(:keys) { %w(foo bar) }
it { is_expected.to eq(2) }
it { is_expected.to eq(2) }
it 'deletes the given keys from the cache' do
subject
it 'deletes the given keys from the cache' do
subject
expect(cache.read(:foo)).to be_empty
expect(cache.read(:bar)).to be_empty
end
expect(cache.read(:foo)).to be_empty
expect(cache.read(:bar)).to be_empty
end
end
context 'no keys' do
let(:keys) { [] }
context 'no keys' do
let(:keys) { [] }
it { is_expected.to eq(0) }
end
it { is_expected.to eq(0) }
end
context 'when deleting over 1000 keys' do
it 'deletes in batches of 1000' do
Gitlab::Redis::RepositoryCache.with do |redis|
expect(redis).to receive(:pipelined).at_least(2).and_call_original
# In a Redis Cluster, we do not want a pipeline to have too many keys
# but in a standalone Redis, multi-key commands can be used.
if ::Gitlab::Redis::ClusterUtil.cluster?(redis)
expect(redis).to receive(:pipelined).at_least(2).and_call_original
else
expect(redis).to receive(:unlink).and_call_original
end
end
cache.expire(*(Array.new(1001) { |i| i }))
end
end
context 'when feature flag is disabled' do
before do
stub_feature_flags(use_pipeline_over_multikey: false)
end
it_behaves_like 'expires varying amount of keys'
end
it_behaves_like 'expires varying amount of keys'
end
describe '#exist?' do
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment