diff options
author | Stan Hu <stanhu@gmail.com> | 2019-03-16 23:23:11 -0700 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2019-03-27 14:46:39 -0500 |
commit | db759c5d9ca3ba9c1610b05d6725c1427d653bef (patch) | |
tree | 08fedc19236dd8c0c317179c93c7ca09f228eeff | |
parent | c624c61a0c65d9b61c06579840d3fb11207b20e7 (diff) | |
download | gitlab-ce-db759c5d9ca3ba9c1610b05d6725c1427d653bef.tar.gz |
Allow ref name caching CommitService#find_commit
For a given merge request, it's quite common to see duplicate FindCommit
Gitaly requests because the Gitaly CommitService caches the request by
the commit SHA, not by the ref name. However, most of the duplicate
requests use the ref name, so the cache is never actually used in
practice. This leads to unnecessary requests that slow performance.
This commit allows certain callers to bypass the ref name to
OID conversion in the cache. We don't do this by default because it's
possible the tip of the branch changes during the commit, which
would cause the caller to get stale data.
This commit also forces the Ci::Pipeline to use the full ref name
so that caching can work for merge requests.
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/57083
-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( |