From 49392fb452cbb6058190f077efe042c6ba9a3067 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 11 Dec 2019 15:05:30 +0100 Subject: [PATCH 1/2] Handle FailedPrecondition for repository languages Repository language detection is expensive, and as such Gitaly might limit the number of GRPC::OK responses GitLab-Rails will receive as part of: https://gitlab.com/gitlab-org/gitaly/merge_requests/1690. This is expected to have following effects: - Repository language detection is considered best effort data and can be stale - The most active repositories are merging to the default branch the most, and will not be scanned as often, decreasing the call rate - The 99 percentile for the Gitaly RPC will stay the same, though the median will fall - The SLA will drop for this RPC, as FailedPrecondition is now returned a lot Additionally, as a small refactor, I've removed support to Gitaly side ref finding when Rails passes an empty ref. This was not used, except in the tests once. --- .../detect_repository_languages_service.rb | 1 + .../detect_repository_languages_worker.rb | 4 +--- lib/gitlab/git/repository.rb | 2 +- lib/gitlab/language_detection.rb | 8 ++++++++ spec/lib/gitlab/git/repository_spec.rb | 2 +- spec/lib/gitlab/language_detection_spec.rb | 18 ++++++++++++++++++ 6 files changed, 30 insertions(+), 5 deletions(-) diff --git a/app/services/projects/detect_repository_languages_service.rb b/app/services/projects/detect_repository_languages_service.rb index d3680637217705..811bdfa116e5db 100644 --- a/app/services/projects/detect_repository_languages_service.rb +++ b/app/services/projects/detect_repository_languages_service.rb @@ -8,6 +8,7 @@ class DetectRepositoryLanguagesService < BaseService def execute repository_languages = project.repository_languages detection = Gitlab::LanguageDetection.new(repository, repository_languages) + return unless dectection.detect! matching_programming_languages = ensure_programming_languages(detection) diff --git a/app/workers/detect_repository_languages_worker.rb b/app/workers/detect_repository_languages_worker.rb index 954d0f9336bd82..71ee3ab2a39d74 100644 --- a/app/workers/detect_repository_languages_worker.rb +++ b/app/workers/detect_repository_languages_worker.rb @@ -12,16 +12,14 @@ class DetectRepositoryLanguagesWorker attr_reader :project - # rubocop: disable CodeReuse/ActiveRecord def perform(project_id, user_id = nil) - @project = Project.find_by(id: project_id) + @project = Project.find_by_id(project_id) return unless project try_obtain_lease do ::Projects::DetectRepositoryLanguagesService.new(project).execute end end - # rubocop: enable CodeReuse/ActiveRecord private diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 4971a18e270800..9ac0f6e70f2165 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -727,7 +727,7 @@ def attributes_at(ref) AttributesAtRefParser.new(self, ref) end - def languages(ref = nil) + def languages(ref) wrapped_gitaly_errors do gitaly_commit_client.languages(ref) end diff --git a/lib/gitlab/language_detection.rb b/lib/gitlab/language_detection.rb index 7600e60b904e5d..402fd72c9fc0f6 100644 --- a/lib/gitlab/language_detection.rb +++ b/lib/gitlab/language_detection.rb @@ -9,6 +9,14 @@ def initialize(repository, repository_languages) @repository_languages = repository_languages end + # Gitaly will not respond to more than 1 + def detect! + detection + true + rescue GRPC::FailedPrecondition + false + end + def languages detection.keys end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 20a74af7a45a8d..40c524a8d924c0 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1503,7 +1503,7 @@ def commit_files(commit) end it "uses the repository's HEAD when no ref is passed" do - lang = repository.languages.first + lang = repository.languages('HEAD').first expect(lang[:label]).to eq('Ruby') end diff --git a/spec/lib/gitlab/language_detection_spec.rb b/spec/lib/gitlab/language_detection_spec.rb index f558ce0d527bdb..afdb24f330187a 100644 --- a/spec/lib/gitlab/language_detection_spec.rb +++ b/spec/lib/gitlab/language_detection_spec.rb @@ -25,6 +25,24 @@ allow(repository).to receive(:languages).and_return(detection) end + describe '#detect!' do + context 'when Gitaly returns results' do + it 'returns true' do + expect(subject.detect!).to eq(true) + end + end + + context 'when Gitaly raises a FailedPrecondition' do + before do + allow(repository).to receive(:languages).and_raise(GRPC::FailedPrecondition) + end + + it 'returns false' do + expect(subject.detect!).to eq(false) + end + end + end + describe '#languages' do it 'returns the language names' do expect(subject.languages).to eq(%w[Ruby JavaScript Elixir CoffeeScript Go]) -- GitLab From 4b428d7f456573d6841efe59412e5f290322e300 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Fri, 13 Dec 2019 09:54:35 +0000 Subject: [PATCH 2/2] Apply suggestion to lib/gitlab/language_detection.rb --- lib/gitlab/language_detection.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/language_detection.rb b/lib/gitlab/language_detection.rb index 402fd72c9fc0f6..b939cfd9430a25 100644 --- a/lib/gitlab/language_detection.rb +++ b/lib/gitlab/language_detection.rb @@ -9,7 +9,8 @@ def initialize(repository, repository_languages) @repository_languages = repository_languages end - # Gitaly will not respond to more than 1 + # Gitaly will raise a FailedPrecondition if the language detection is fresh enough it's not worth the effort to + # detect again. def detect! detection true -- GitLab