summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorTiago Botelho <tiagonbotelho@hotmail.com>2018-10-01 13:29:47 +0100
committerTiago Botelho <tiagonbotelho@hotmail.com>2018-10-01 13:29:47 +0100
commita583f137ddc9833796752b9cf8c07077d76078ba (patch)
treecc92c76a4d29f61a24740e5ab45cd71926677d9c
parente8e1a51adddd9d7a504c06ea383cf682493f6a90 (diff)
downloadgitlab-ce-a583f137ddc9833796752b9cf8c07077d76078ba.tar.gz
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
-rw-r--r--GITALY_SERVER_VERSION2
-rw-r--r--Gemfile2
-rw-r--r--Gemfile.lock4
-rw-r--r--Gemfile.rails5.lock4
-rw-r--r--app/models/repository.rb8
-rw-r--r--changelogs/unreleased/37433-solve-n-1-in-refs-controller-logs-tree.yml6
-rw-r--r--lib/gitlab/git/repository.rb6
-rw-r--r--lib/gitlab/gitaly_client/commit_service.rb18
-rw-r--r--lib/gitlab/tree_summary.rb28
-rw-r--r--spec/models/repository_spec.rb51
10 files changed, 111 insertions, 18 deletions
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index ee1e4d2aee5..3ba7bd5ba83 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-0.122.0
+0.123.0
diff --git a/Gemfile b/Gemfile
index 35e83a530f0..dcb97d1e7ad 100644
--- a/Gemfile
+++ b/Gemfile
@@ -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
diff --git a/Gemfile.lock b/Gemfile.lock
index d8eaaac99b1..8e8abb05a67 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -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)
diff --git a/Gemfile.rails5.lock b/Gemfile.rails5.lock
index ab35a4a399f..a7728af0ebe 100644
--- a/Gemfile.rails5.lock
+++ b/Gemfile.rails5.lock
@@ -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)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 4fecdb3c1ad..a3a3ce179fc 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -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
diff --git a/changelogs/unreleased/37433-solve-n-1-in-refs-controller-logs-tree.yml b/changelogs/unreleased/37433-solve-n-1-in-refs-controller-logs-tree.yml
new file mode 100644
index 00000000000..04662a7cfe2
--- /dev/null
+++ b/changelogs/unreleased/37433-solve-n-1-in-refs-controller-logs-tree.yml
@@ -0,0 +1,6 @@
+---
+title: Adds support for Gitaly ListLastCommitsForTree RPC in order to make bulk-fetch
+ of commits more performant
+merge_request: 21921
+author:
+type: performance
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 3d5a63bdbac..45d42c7078f 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -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)
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index 07e5e204b68..085b2a127a5 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -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,
diff --git a/lib/gitlab/tree_summary.rb b/lib/gitlab/tree_summary.rb
index b05d408b1c0..c2955cd374c 100644
--- a/lib/gitlab/tree_summary.rb
+++ b/lib/gitlab/tree_summary.rb
@@ -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)
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 7e1b7c35517..784d17e271e 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -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 }