Commit a583f137 authored by Tiago Botelho's avatar Tiago Botelho

Removes N+1 gitaly rpc call to fetch the last commit for path

Implements list_last_commits_for_tree to communicate with the
ListLastCommitsForTree Gitaly RPC

Bumps the Gitaly server version

Bumps the Gitaly-Proto gem version
parent e8e1a51a
......@@ -423,7 +423,7 @@ group :ed25519 do
end
# Gitaly GRPC client
gem 'gitaly-proto', '~> 0.117.0', require: 'gitaly'
gem 'gitaly-proto', '~> 0.118.1', require: 'gitaly'
gem 'grpc', '~> 1.11.0'
# Locked until https://github.com/google/protobuf/issues/4210 is closed
......
......@@ -276,7 +276,7 @@ GEM
gettext_i18n_rails (>= 0.7.1)
po_to_json (>= 1.0.0)
rails (>= 3.2.0)
gitaly-proto (0.117.0)
gitaly-proto (0.118.1)
google-protobuf (~> 3.1)
grpc (~> 1.10)
github-linguist (5.3.3)
......@@ -1031,7 +1031,7 @@ DEPENDENCIES
gettext (~> 3.2.2)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
gitaly-proto (~> 0.117.0)
gitaly-proto (~> 0.118.1)
github-linguist (~> 5.3.3)
gitlab-flowdock-git-hook (~> 1.0.1)
gitlab-gollum-lib (~> 4.2)
......
......@@ -279,7 +279,7 @@ GEM
gettext_i18n_rails (>= 0.7.1)
po_to_json (>= 1.0.0)
rails (>= 3.2.0)
gitaly-proto (0.117.0)
gitaly-proto (0.118.1)
google-protobuf (~> 3.1)
grpc (~> 1.10)
github-linguist (5.3.3)
......@@ -1040,7 +1040,7 @@ DEPENDENCIES
gettext (~> 3.2.2)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
gitaly-proto (~> 0.117.0)
gitaly-proto (~> 0.118.1)
github-linguist (~> 5.3.3)
gitlab-flowdock-git-hook (~> 1.0.1)
gitlab-gollum-lib (~> 4.2)
......
......@@ -668,6 +668,14 @@ class Repository
end
end
def list_last_commits_for_tree(sha, path, offset: 0, limit: 25)
commits = raw_repository.list_last_commits_for_tree(sha, path, offset: offset, limit: limit)
commits.each do |path, commit|
commits[path] = ::Commit.new(commit, @project)
end
end
def last_commit_for_path(sha, path)
commit = raw_repository.last_commit_for_path(sha, path)
::Commit.new(commit, @project) if commit
......
---
title: Adds support for Gitaly ListLastCommitsForTree RPC in order to make bulk-fetch
of commits more performant
merge_request: 21921
author:
type: performance
......@@ -953,6 +953,12 @@ module Gitlab
end
end
def list_last_commits_for_tree(sha, path, offset: 0, limit: 25)
wrapped_gitaly_errors do
gitaly_commit_client.list_last_commits_for_tree(sha, path, offset: offset, limit: limit)
end
end
def last_commit_for_path(sha, path)
wrapped_gitaly_errors do
gitaly_commit_client.last_commit_for_path(sha, path)
......
......@@ -148,6 +148,24 @@ module Gitlab
GitalyClient.call(@repository.storage, :commit_service, :count_commits, request, timeout: GitalyClient.medium_timeout).count
end
def list_last_commits_for_tree(revision, path, offset: 0, limit: 25)
request = Gitaly::ListLastCommitsForTreeRequest.new(
repository: @gitaly_repo,
revision: encode_binary(revision),
path: encode_binary(path.to_s),
offset: offset,
limit: limit
)
response = GitalyClient.call(@repository.storage, :commit_service, :list_last_commits_for_tree, request, timeout: GitalyClient.medium_timeout)
response.each_with_object({}) do |gitaly_response, hsh|
gitaly_response.commits.each do |commit_for_tree|
hsh[commit_for_tree.path] = Gitlab::Git::Commit.new(@repository, commit_for_tree.commit)
end
end
end
def last_commit_for_path(revision, path)
request = Gitaly::LastCommitForPathRequest.new(
repository: @gitaly_repo,
......
......@@ -73,25 +73,29 @@ module Gitlab
end
def fill_last_commits!(entries)
# n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37433
Gitlab::GitalyClient.allow_n_plus_1_calls do
entries.each do |entry|
raw_commit = repository.last_commit_for_path(commit.id, entry_path(entry))
# Ensure the path is in "path/" format
ensured_path =
if path
File.join(*[path, ""])
end
commits_hsh = repository.list_last_commits_for_tree(commit.id, ensured_path, offset: offset, limit: limit)
if raw_commit
commit = resolve_commit(raw_commit)
entries.each do |entry|
path_key = entry_path(entry)
commit = cache_commit(commits_hsh[path_key])
entry[:commit] = commit
entry[:commit_path] = commit_path(commit)
end
if commit
entry[:commit] = commit
entry[:commit_path] = commit_path(commit)
end
end
end
def resolve_commit(raw_commit)
return nil unless raw_commit.present?
def cache_commit(commit)
return nil unless commit.present?
resolved_commits[raw_commit.id] ||= ::Commit.new(raw_commit, project)
resolved_commits[commit.id] ||= commit
end
def commit_path(commit)
......
......@@ -188,6 +188,57 @@ describe Repository do
end
end
describe '#list_last_commits_for_tree' do
let(:path_to_commit) do
{
"encoding" => "913c66a37b4a45b9769037c55c2d238bd0942d2e",
"files" => "570e7b2abdd848b95f2f578043fc23bd6f6fd24d",
".gitignore" => "c1acaa58bbcbc3eafe538cb8274ba387047b69f8",
".gitmodules" => "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9",
"CHANGELOG" => "913c66a37b4a45b9769037c55c2d238bd0942d2e",
"CONTRIBUTING.md" => "6d394385cf567f80a8fd85055db1ab4c5295806f",
"Gemfile.zip" => "ae73cb07c9eeaf35924a10f713b364d32b2dd34f",
"LICENSE" => "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863",
"MAINTENANCE.md" => "913c66a37b4a45b9769037c55c2d238bd0942d2e",
"PROCESS.md" => "913c66a37b4a45b9769037c55c2d238bd0942d2e",
"README.md" => "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863",
"VERSION" => "913c66a37b4a45b9769037c55c2d238bd0942d2e",
"gitlab-shell" => "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9",
"six" => "cfe32cf61b73a0d5e9f13e774abde7ff789b1660"
}
end
subject { repository.list_last_commits_for_tree(sample_commit.id, '.').id }
it 'returns the last commits for every entry in the current path' do
result = repository.list_last_commits_for_tree(sample_commit.id, '.')
result.each do |key, value|
result[key] = value.id
end
expect(result).to include(path_to_commit)
end
it 'returns the last commits for every entry in the current path starting from the offset' do
result = repository.list_last_commits_for_tree(sample_commit.id, '.', offset: path_to_commit.size - 1)
expect(result.size).to eq(1)
end
it 'returns a limited number of last commits for every entry in the current path starting from the offset' do
result = repository.list_last_commits_for_tree(sample_commit.id, '.', limit: 1)
expect(result.size).to eq(1)
end
it 'returns an empty hash when offset is out of bounds' do
result = repository.list_last_commits_for_tree(sample_commit.id, '.', offset: path_to_commit.size)
expect(result.size).to eq(0)
end
end
describe '#last_commit_for_path' do
shared_examples 'getting last commit for path' do
subject { repository.last_commit_for_path(sample_commit.id, '.gitignore').id }
......
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