Commit a39ad2a3 authored by Stan Hu's avatar Stan Hu

Remove merge_request_eager_fetch_ref feature flag

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80876 fixed a
problem where the Gitaly `FetchSourceBranch` RPC would cause the
database to idle in transaction. This happened because the call was
made in an `after_save` hook, which wraps everything in a transaction.
On GitLab.com, the 15-second idle in transaction timeout would close
the connection, but this was only apparent to the application when it
attempted to write a merge request metrics entry for a non-existent
merge request, resulting in a foreign key violation.

With the feature flag enabled, the `FetchSourceBranch` occurs outside
this hook. Since enabling this feature flag, we no longer see this
idle in transaction problem.

Relates to:

1. https://gitlab.com/gitlab-org/gitlab/-/issues/336657
2. https://gitlab.com/gitlab-org/gitlab/-/issues/353044
parent 59440b13
...@@ -1026,12 +1026,13 @@ class MergeRequest < ApplicationRecord ...@@ -1026,12 +1026,13 @@ class MergeRequest < ApplicationRecord
ensure_target_project_iid! ensure_target_project_iid!
fetch_ref! fetch_ref!
# Prevent the after_create hook from fetching the source branch again # Prevent the after_create hook from fetching the source branch again.
# Drop this field after rollout in https://gitlab.com/gitlab-org/gitlab/-/issues/353044.
@skip_fetch_ref = true @skip_fetch_ref = true
end end
def create_merge_request_diff def create_merge_request_diff
# Callers such as MergeRequests::BuildService may not call eager_fetch_ref!. Just
# in case they haven't, we fetch the ref.
fetch_ref! unless skip_fetch_ref fetch_ref! unless skip_fetch_ref
# n+1: https://gitlab.com/gitlab-org/gitlab/-/issues/19377 # n+1: https://gitlab.com/gitlab-org/gitlab/-/issues/19377
......
...@@ -36,9 +36,7 @@ module MergeRequests ...@@ -36,9 +36,7 @@ module MergeRequests
# callback (e.g. after_create), a database transaction will be # callback (e.g. after_create), a database transaction will be
# open while the Gitaly RPC waits. To avoid an idle in transaction # open while the Gitaly RPC waits. To avoid an idle in transaction
# timeout, we do this before we attempt to save the merge request. # timeout, we do this before we attempt to save the merge request.
if Feature.enabled?(:merge_request_eager_fetch_ref, @project, default_enabled: :yaml) merge_request.eager_fetch_ref!
merge_request.eager_fetch_ref!
end
end end
def set_projects! def set_projects!
......
---
name: merge_request_eager_fetch_ref
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80876
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/353044
milestone: '14.9'
type: development
group: group::code review
default_enabled: false
...@@ -454,7 +454,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do ...@@ -454,7 +454,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
end end
end end
shared_examples 'when source and target projects are different' do |eager_fetch_ref_enabled| shared_examples 'when source and target projects are different' do
let(:target_project) { fork_project(project, nil, repository: true) } let(:target_project) { fork_project(project, nil, repository: true) }
let(:opts) do let(:opts) do
...@@ -498,11 +498,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do ...@@ -498,11 +498,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
it 'creates the merge request', :sidekiq_might_not_need_inline do it 'creates the merge request', :sidekiq_might_not_need_inline do
expect_next_instance_of(MergeRequest) do |instance| expect_next_instance_of(MergeRequest) do |instance|
if eager_fetch_ref_enabled expect(instance).to receive(:eager_fetch_ref!).and_call_original
expect(instance).to receive(:eager_fetch_ref!).and_call_original
else
expect(instance).not_to receive(:eager_fetch_ref!)
end
end end
merge_request = described_class.new(project: project, current_user: user, params: opts).execute merge_request = described_class.new(project: project, current_user: user, params: opts).execute
...@@ -520,17 +516,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do ...@@ -520,17 +516,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
end end
end end
context 'when merge_request_eager_fetch_ref is enabled' do it_behaves_like 'when source and target projects are different'
it_behaves_like 'when source and target projects are different', true
end
context 'when merge_request_eager_fetch_ref is disabled' do
before do
stub_feature_flags(merge_request_eager_fetch_ref: false)
end
it_behaves_like 'when source and target projects are different', false
end
context 'when user sets source project id' do context 'when user sets source project id' do
let(:another_project) { create(:project) } let(:another_project) { create(:project) }
......
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