diff options
| -rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 4 | ||||
| -rw-r--r-- | app/models/ci/pipeline.rb | 26 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-fix-gitaly-find-commit-caching.yml | 5 | ||||
| -rw-r--r-- | lib/gitlab/gitaly_client.rb | 19 | ||||
| -rw-r--r-- | lib/gitlab/gitaly_client/commit_service.rb | 2 | ||||
| -rw-r--r-- | spec/controllers/projects/merge_requests_controller_spec.rb | 4 | ||||
| -rw-r--r-- | spec/lib/gitlab/gitaly_client/commit_service_spec.rb | 15 | ||||
| -rw-r--r-- | spec/models/ci/pipeline_spec.rb | 8 |
8 files changed, 69 insertions, 14 deletions
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 5cf7fa3422d..34cb0416965 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -315,7 +315,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo end def serializer - MergeRequestSerializer.new(current_user: current_user, project: merge_request.project) + ::Gitlab::GitalyClient.allow_ref_name_caching do + MergeRequestSerializer.new(current_user: current_user, project: merge_request.project) + end end def define_edit_vars diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 826b3f82bbf..bbde0cdd465 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -465,9 +465,9 @@ module Ci end def latest? - return false unless ref && commit.present? + return false unless git_ref && commit.present? - project.commit(ref) == commit + project.commit(git_ref) == commit end def retried @@ -781,16 +781,18 @@ module Ci end def git_ref - if merge_request_event? - ## - # In the future, we're going to change this ref to - # merge request's merged reference, such as "refs/merge-requests/:iid/merge". - # In order to do that, we have to update GitLab-Runner's source pulling - # logic. - # See https://gitlab.com/gitlab-org/gitlab-runner/merge_requests/1092 - Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s - else - super + strong_memoize(:git_pref) do + if merge_request_event? + ## + # In the future, we're going to change this ref to + # merge request's merged reference, such as "refs/merge-requests/:iid/merge". + # In order to do that, we have to update GitLab-Runner's source pulling + # logic. + # See https://gitlab.com/gitlab-org/gitlab-runner/merge_requests/1092 + Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s + else + super + end end end diff --git a/changelogs/unreleased/sh-fix-gitaly-find-commit-caching.yml b/changelogs/unreleased/sh-fix-gitaly-find-commit-caching.yml new file mode 100644 index 00000000000..16d349c407c --- /dev/null +++ b/changelogs/unreleased/sh-fix-gitaly-find-commit-caching.yml @@ -0,0 +1,5 @@ +--- +title: Allow ref name caching CommitService#find_commit +merge_request: 26248 +author: +type: performance diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index e3b9a7a1a89..bc2f7693fdc 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -296,6 +296,25 @@ module Gitlab end end + # Normally a FindCommit RPC will cache the commit with its SHA + # instead of a ref name, since it's possible the branch is mutated + # afterwards. However, for read-only requests that never mutate the + # branch, this method allows caching of the ref name directly. + def self.allow_ref_name_caching + return yield unless Gitlab::SafeRequestStore.active? + + begin + Gitlab::SafeRequestStore[:allow_ref_name_caching] = true + yield + ensure + Gitlab::SafeRequestStore[:allow_ref_name_caching] = false + end + end + + def self.ref_name_caching_allowed? + Gitlab::SafeRequestStore[:allow_ref_name_caching] + end + def self.get_call_count(key) Gitlab::SafeRequestStore[key] || 0 end diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index ea12424eb4a..0d5debfcd01 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -286,7 +286,7 @@ module Gitlab commit = call_find_commit(revision) return unless commit - key[:commit_id] = commit.id + key[:commit_id] = commit.id unless GitalyClient.ref_name_caching_allowed? Gitlab::SafeRequestStore[key] = commit else call_find_commit(revision) diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 79f97aa4170..c8fa93a74ee 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -86,6 +86,10 @@ describe Projects::MergeRequestsController do end describe 'as json' do + before do + expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original + end + context 'with basic serializer param' do it 'renders basic MR entity as json' do go(serializer: 'basic', format: :json) diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb index d7bd757149d..6d6107ca3e7 100644 --- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb @@ -221,6 +221,21 @@ describe Gitlab::GitalyClient::CommitService do expect(commit).to eq(commit_dbl) end end + + context 'when caching of the ref name is enabled' do + it 'returns a cached commit' do + expect_any_instance_of(Gitaly::CommitService::Stub).to receive(:find_commit).once.and_return(double(commit: commit_dbl)) + + commit = nil + 2.times do + ::Gitlab::GitalyClient.allow_ref_name_caching do + commit = described_class.new(repository).find_commit('master') + end + end + + expect(commit).to eq(commit_dbl) + end + end end end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 5b8097621e0..225c6812bec 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1460,6 +1460,14 @@ describe Ci::Pipeline, :mailer do end end + context 'with a branch name as the ref' do + it 'looks up commit with the full ref name' do + expect(pipeline.project).to receive(:commit).with('refs/heads/master').and_call_original + + expect(pipeline).to be_latest + end + end + context 'with not latest sha' do before do pipeline.update( |