Merge branch 'fix/cache-gitaly-find-commits' into 'master'

Cache Gitaly FindCommit RPC response

See merge request gitlab-org/gitlab-ce!16949

lib/gitlab/gitaly_client/commit_service.rb

@@ -222,14 +222,25 @@ module Gitlab
       end
 
       def find_commit(revision)
-        request = Gitaly::FindCommitRequest.new(
+        if RequestStore.active?
-          repository: @gitaly_repo,
+          # We don't use RequeStstore.fetch(key) { ... } directly because `revision`
-          revision: encode_binary(revision)
+          # can be a branch name, so we can't use it as a key as it could point
-        )
+          # to another commit later on (happens a lot in tests).
+          key = {
+            storage: @gitaly_repo.storage_name,
+            relative_path: @gitaly_repo.relative_path,
+            commit_id: revision
+          }
+          return RequestStore[key] if RequestStore.exist?(key)
 
-        response = GitalyClient.call(@repository.storage, :commit_service, :find_commit, request, timeout: GitalyClient.medium_timeout)
+          commit = call_find_commit(revision)
+          return unless commit
 
-        response.commit
+          key[:commit_id] = commit.id
+          RequestStore[key] = commit
+        else
+          call_find_commit(revision)
+        end
       end
 
       def patch(revision)
@@ -346,6 +357,17 @@ module Gitlab
       def encode_repeated(a)
         Google::Protobuf::RepeatedField.new(:bytes, a.map { |s| encode_binary(s) } )
       end
+
+      def call_find_commit(revision)
+        request = Gitaly::FindCommitRequest.new(
+          repository: @gitaly_repo,
+          revision: encode_binary(revision)
+        )
+
+        response = GitalyClient.call(@repository.storage, :commit_service, :find_commit, request, timeout: GitalyClient.medium_timeout)
+
+        response.commit
+      end
     end
   end
 end

spec/lib/gitlab/gitaly_client/commit_service_spec.rb

@@ -166,6 +166,32 @@ describe Gitlab::GitalyClient::CommitService do
 
       described_class.new(repository).find_commit(revision)
     end
+
+    describe 'caching', :request_store do
+      let(:commit_dbl) { double(id: 'f01b' * 10) }
+
+      context 'when passed revision is a branch name' do
+        it 'calls Gitaly' do
+          expect_any_instance_of(Gitaly::CommitService::Stub).to receive(:find_commit).twice.and_return(double(commit: commit_dbl))
+
+          commit = nil
+          2.times { commit = described_class.new(repository).find_commit('master') }
+
+          expect(commit).to eq(commit_dbl)
+        end
+      end
+
+      context 'when passed revision is a commit ID' 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 { commit = described_class.new(repository).find_commit('f01b' * 10) }
+
+          expect(commit).to eq(commit_dbl)
+        end
+      end
+    end
   end
 
   describe '#patch' do