Commit 84a4415e authored by Kerri Miller's avatar Kerri Miller

Add param to allow scoped caching of Repo#merge_to_ref

Adding caching to this endpoint for all use cases triggered a bug in an
untested portion of Merge Trains. Adding this param will allow us to
selectively apply caching, improving the performance characteristics of
where we have a problem (MergeRequests::MergeabilityCheckService) but
not in the problematic Merge Trains code.

Changelog: added
parent fed187a7
...@@ -13,12 +13,12 @@ module MergeRequests ...@@ -13,12 +13,12 @@ module MergeRequests
class MergeToRefService < MergeRequests::MergeBaseService class MergeToRefService < MergeRequests::MergeBaseService
extend ::Gitlab::Utils::Override extend ::Gitlab::Utils::Override
def execute(merge_request) def execute(merge_request, cache_merge_to_ref_calls = false)
@merge_request = merge_request @merge_request = merge_request
error_check! error_check!
commit_id = commit commit_id = commit(cache_merge_to_ref_calls)
raise_error('Conflicts detected during merge') unless commit_id raise_error('Conflicts detected during merge') unless commit_id
...@@ -65,8 +65,9 @@ module MergeRequests ...@@ -65,8 +65,9 @@ module MergeRequests
params[:allow_conflicts] || false params[:allow_conflicts] || false
end end
def commit def commit(cache_merge_to_ref_calls = false)
if Feature.enabled?(:cache_merge_to_ref_calls, project, default_enabled: false) if cache_merge_to_ref_calls &&
Feature.enabled?(:cache_merge_to_ref_calls, project, default_enabled: false)
Rails.cache.fetch(cache_key, expires_in: 1.day) do Rails.cache.fetch(cache_key, expires_in: 1.day) do
extracted_merge_to_ref extracted_merge_to_ref
end end
......
...@@ -43,12 +43,22 @@ RSpec.describe MergeRequests::MergeToRefService do ...@@ -43,12 +43,22 @@ RSpec.describe MergeRequests::MergeToRefService do
# warm the cache # warm the cache
# #
service.execute(merge_request) service.execute(merge_request, true)
end
context 'when cache_merge_to_ref_calls parameter is true' do
it 'caches the response', :request_store do
expect { 3.times { service.execute(merge_request, true) } }
.not_to change(Gitlab::GitalyClient, :get_request_count)
end
end end
it 'caches the response', :request_store do context 'when cache_merge_to_ref_calls parameter is false' do
expect { 3.times { service.execute(merge_request) } } it 'does not cache the response', :request_store do
.not_to change(Gitlab::GitalyClient, :get_request_count) expect(Gitlab::GitalyClient).to receive(:call).at_least(3).times.and_call_original
3.times { service.execute(merge_request, false) }
end
end end
end end
...@@ -58,13 +68,15 @@ RSpec.describe MergeRequests::MergeToRefService do ...@@ -58,13 +68,15 @@ RSpec.describe MergeRequests::MergeToRefService do
# warm the cache # warm the cache
# #
service.execute(merge_request) service.execute(merge_request, true)
end end
it 'does not cache the response', :request_store do [true, false].each do |cache_merge_to_ref_calls|
expect(Gitlab::GitalyClient).to receive(:call).at_least(3).times.and_call_original it 'does not cache the response, regardless of cache_merge_to_ref_calls state', :request_store do
expect(Gitlab::GitalyClient).to receive(:call).at_least(3).times.and_call_original
3.times { service.execute(merge_request) } 3.times { service.execute(merge_request, cache_merge_to_ref_calls) }
end
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment