Commit 295f2e38 authored by Patrick Steinhardt's avatar Patrick Steinhardt

checks: Always enable batched computation of commits

With 156ce433 (checks: Implement infrastructure for bulk diff checks,
2021-07-29), we have implemented batched loading of all commits for
access checks in contrast to loading commits for each change. This both
enables us to do a single RPC call to fetch all changes, and furthermore
it allows us to use the quarantine directory to enumerate new commits
instead of doing a revision walk. Together, these are a big performance
improvement based on how many references a repository has: for
gitlab-org/gitlab, the time to compute new commits went down from 19
seconds to about 12 milliseconds.

Remove the feature flag of this important performance optimization. The
feature flag had been enabled in production on August 16th and can thus
be deemed stable.

Changelog: performance
parent 647312f4
---
name: changes_batch_commits
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66731
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/336992
milestone: '14.2'
type: development
group: group::gitaly
default_enabled: false
...@@ -81,7 +81,7 @@ module Gitlab ...@@ -81,7 +81,7 @@ module Gitlab
def single_access_checks! def single_access_checks!
# Iterate over all changes to find if user allowed all of them to be applied # Iterate over all changes to find if user allowed all of them to be applied
changes.each do |change| changes.each do |change|
commits = Gitlab::Lazy.new { commits_for(change[:newrev]) } if Feature.enabled?(:changes_batch_commits) commits = Gitlab::Lazy.new { commits_for(change[:newrev]) }
# If user does not have access to make at least one change, cancel all # If user does not have access to make at least one change, cancel all
# push by allowing the exception to bubble up # push by allowing the exception to bubble up
......
...@@ -8,53 +8,35 @@ RSpec.describe Gitlab::Checks::ChangesAccess do ...@@ -8,53 +8,35 @@ RSpec.describe Gitlab::Checks::ChangesAccess do
subject { changes_access } subject { changes_access }
describe '#validate!' do describe '#validate!' do
shared_examples '#validate!' do before do
before do allow(project).to receive(:lfs_enabled?).and_return(true)
allow(project).to receive(:lfs_enabled?).and_return(true) end
end
context 'without failed checks' do
it "doesn't raise an error" do
expect { subject.validate! }.not_to raise_error
end
it 'calls lfs checks' do
expect_next_instance_of(Gitlab::Checks::LfsCheck) do |instance|
expect(instance).to receive(:validate!)
end
subject.validate! context 'without failed checks' do
end it "doesn't raise an error" do
expect { subject.validate! }.not_to raise_error
end end
context 'when time limit was reached' do it 'calls lfs checks' do
it 'raises a TimeoutError' do expect_next_instance_of(Gitlab::Checks::LfsCheck) do |instance|
logger = Gitlab::Checks::TimedLogger.new(start_time: timeout.ago, timeout: timeout) expect(instance).to receive(:validate!)
access = described_class.new(changes,
project: project,
user_access: user_access,
protocol: protocol,
logger: logger)
expect { access.validate! }.to raise_error(Gitlab::Checks::TimedLogger::TimeoutError)
end end
end
end
context 'with batched commits enabled' do subject.validate!
before do
stub_feature_flags(changes_batch_commits: true)
end end
it_behaves_like '#validate!'
end end
context 'with batched commits disabled' do context 'when time limit was reached' do
before do it 'raises a TimeoutError' do
stub_feature_flags(changes_batch_commits: false) logger = Gitlab::Checks::TimedLogger.new(start_time: timeout.ago, timeout: timeout)
end access = described_class.new(changes,
project: project,
user_access: user_access,
protocol: protocol,
logger: logger)
it_behaves_like '#validate!' expect { access.validate! }.to raise_error(Gitlab::Checks::TimedLogger::TimeoutError)
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