Commit 1993feb5 authored by Stan Hu's avatar Stan Hu

Optimize JIRA ref lookup

In a project with JIRA activated, `ProcessCommitWorker` attempts to add
a comment to a JIRA issue if that issue is mentioned in a
commit. However, the JIRA integration would attempt to retrieve all tags
and branches and pick the first matching ref given a commit OID.

The problem with that approach is that after each push, the list of all
branches and tags are expired and could take a while to gather. Since
multiple `ProcessCommitWorker` jobs can be running at the same time,
this can lead to high I/O on Gitaly nodes since multiple
`ProcessCommitWorker` jobs can run at the same time.

We observe that we don't really need to build the entire ref list; we
can just ask Gitaly for a single matching ref for the given OID with the
newly-created `FindRefsByOID` RPC introduced in
https://gitlab.com/gitlab-org/gitaly/-/merge_requests/3947.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/343035

Changelog: performance
parent 8dbc57a9
......@@ -476,7 +476,7 @@ end
gem 'spamcheck', '~> 0.1.0'
# Gitaly GRPC protocol definitions
gem 'gitaly', '~> 14.3.0.pre.rc2'
gem 'gitaly', '~> 14.4.0.pre.rc43'
# KAS GRPC protocol definitions
gem 'kas-grpc', '~> 0.0.2'
......
......@@ -450,7 +450,7 @@ GEM
rails (>= 3.2.0)
git (1.7.0)
rchardet (~> 1.8)
gitaly (14.3.0.pre.rc2)
gitaly (14.4.0.pre.rc43)
grpc (~> 1.0)
github-markup (1.7.0)
gitlab (4.16.1)
......@@ -1457,7 +1457,7 @@ DEPENDENCIES
gettext (~> 3.3)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
gitaly (~> 14.3.0.pre.rc2)
gitaly (~> 14.4.0.pre.rc43)
github-markup (~> 1.7.0)
gitlab-chronic (~> 0.10.5)
gitlab-dangerfiles (~> 2.3.1)
......
......@@ -302,6 +302,14 @@ module Integrations
private
def branch_name(noteable)
if Feature.enabled?(:jira_use_first_ref_by_oid, project, default_enabled: :yaml)
noteable.first_ref_by_oid(project.repository)
else
noteable.ref_names(project.repository).first
end
end
def server_info
strong_memoize(:server_info) do
client_url.present? ? jira_request { client.ServerInfo.all.attrs } : nil
......@@ -495,7 +503,7 @@ module Integrations
{
id: noteable.short_id,
description: noteable.safe_message,
branch: noteable.ref_names(project.repository).first
branch: branch_name(noteable)
}
elsif noteable.is_a?(MergeRequest)
{
......
---
name: jira_use_first_ref_by_oid
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72739
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/343585
milestone: '14.5'
type: development
group: group::integrations
default_enabled: false
......@@ -315,10 +315,18 @@ module Gitlab
#
def ref_names(repo)
refs(repo).map do |ref|
ref.sub(%r{^refs/(heads|remotes|tags)/}, "")
strip_ref_prefix(ref)
end
end
def first_ref_by_oid(repo)
ref = repo.refs_by_oid(oid: id, limit: 1)&.first
return unless ref
strip_ref_prefix(ref)
end
def message
encode! @message
end
......@@ -466,6 +474,10 @@ module Gitlab
commit_id.match?(/\s/)
)
end
def strip_ref_prefix(ref)
ref.sub(%r{^refs/(heads|remotes|tags)/}, "")
end
end
end
end
......
......@@ -519,6 +519,17 @@ module Gitlab
@refs_hash
end
# Returns matching refs for OID
#
# Limit of 0 means there is no limit.
def refs_by_oid(oid:, limit: 0)
wrapped_gitaly_errors do
gitaly_ref_client.find_refs_by_oid(oid: oid, limit: limit)
end
rescue CommandError, TypeError, NoRepository
nil
end
# Returns url for submodule
#
# Ex.
......
......@@ -210,6 +210,13 @@ module Gitlab
GitalyClient.call(@storage, :ref_service, :pack_refs, request, timeout: GitalyClient.long_timeout)
end
def find_refs_by_oid(oid:, limit:)
request = Gitaly::FindRefsByOIDRequest.new(repository: @gitaly_repo, sort_field: :refname, oid: oid, limit: limit)
response = GitalyClient.call(@storage, :ref_service, :find_refs_by_oid, request, timeout: GitalyClient.medium_timeout)
response&.refs&.to_a
end
private
def consume_refs_response(response)
......
......@@ -715,6 +715,14 @@ RSpec.describe Gitlab::Git::Commit, :seed_helper do
it { is_expected.not_to include("feature") }
end
describe '#first_ref_by_oid' do
let(:commit) { described_class.find(repository, 'master') }
subject { commit.first_ref_by_oid(repository) }
it { is_expected.to eq("master") }
end
describe '.get_message' do
let(:commit_ids) { %w[6d394385cf567f80a8fd85055db1ab4c5295806f cfe32cf61b73a0d5e9f13e774abde7ff789b1660] }
......
......@@ -1900,6 +1900,32 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
end
end
describe '#refs_by_oid' do
it 'returns a list of refs from a OID' do
refs = repository.refs_by_oid(oid: repository.commit.id)
expect(refs).to be_an(Array)
expect(refs).to include(Gitlab::Git::BRANCH_REF_PREFIX + repository.root_ref)
end
it 'returns a single ref from a OID' do
refs = repository.refs_by_oid(oid: repository.commit.id, limit: 1)
expect(refs).to be_an(Array)
expect(refs).to eq([Gitlab::Git::BRANCH_REF_PREFIX + repository.root_ref])
end
it 'returns empty for unknown ID' do
expect(repository.refs_by_oid(oid: Gitlab::Git::BLANK_SHA, limit: 0)).to eq([])
end
it 'returns nil for an empty repo' do
project = create(:project)
expect(project.repository.refs_by_oid(oid: SeedRepo::Commit::ID, limit: 0)).to be_nil
end
end
describe '#set_full_path' do
before do
repository_rugged.config["gitlab.fullpath"] = repository_path
......
......@@ -282,4 +282,19 @@ RSpec.describe Gitlab::GitalyClient::RefService do
client.pack_refs
end
end
describe '#find_refs_by_oid' do
let(:oid) { project.repository.commit.id }
it 'sends a find_refs_by_oid message' do
expect_any_instance_of(Gitaly::RefService::Stub)
.to receive(:find_refs_by_oid)
.with(gitaly_request_with_params(sort_field: 'refname', oid: oid, limit: 1), kind_of(Hash))
.and_call_original
refs = client.find_refs_by_oid(oid: oid, limit: 1)
expect(refs.to_a).to eq([Gitlab::Git::BRANCH_REF_PREFIX + project.repository.root_ref])
end
end
end
......@@ -876,10 +876,24 @@ RSpec.describe Integrations::Jira do
subject
expect(WebMock).to have_requested(:post, comment_url).with(
body: /mentioned this issue in/
body: /mentioned this issue.*on branch \[master/
).once
end
context 'with jira_use_first_ref_by_oid feature flag disabled' do
before do
stub_feature_flags(jira_use_first_ref_by_oid: false)
end
it 'creates a comment on Jira' do
subject
expect(WebMock).to have_requested(:post, comment_url).with(
body: /mentioned this issue.*on branch \[master/
).once
end
end
it 'tracks usage' do
expect(Gitlab::UsageDataCounters::HLLRedisCounter)
.to receive(:track_event)
......
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