Commit 734b8410 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'cache-refactor' into 'master'

Cache `#can_be_resolved_in_ui?` git operations

Closes gitaly#1051

See merge request gitlab-org/gitlab-ce!17589
parents dad547a5 bb2bf39d
...@@ -16,6 +16,7 @@ class Repository ...@@ -16,6 +16,7 @@ class Repository
].freeze ].freeze
include Gitlab::ShellAdapter include Gitlab::ShellAdapter
include Gitlab::RepositoryCacheAdapter
attr_accessor :full_path, :disk_path, :project, :is_wiki attr_accessor :full_path, :disk_path, :project, :is_wiki
...@@ -57,22 +58,6 @@ class Repository ...@@ -57,22 +58,6 @@ class Repository
merge_request_template: :merge_request_template_names merge_request_template: :merge_request_template_names
}.freeze }.freeze
# Wraps around the given method and caches its output in Redis and an instance
# variable.
#
# This only works for methods that do not take any arguments.
def self.cache_method(name, fallback: nil, memoize_only: false)
original = :"_uncached_#{name}"
alias_method(original, name)
define_method(name) do
cache_method_output(name, fallback: fallback, memoize_only: memoize_only) do
__send__(original) # rubocop:disable GitlabSecurity/PublicSend
end
end
end
def initialize(full_path, project, disk_path: nil, is_wiki: false) def initialize(full_path, project, disk_path: nil, is_wiki: false)
@full_path = full_path @full_path = full_path
@disk_path = disk_path || full_path @disk_path = disk_path || full_path
...@@ -302,17 +287,6 @@ class Repository ...@@ -302,17 +287,6 @@ class Repository
expire_method_caches(CACHED_METHODS) expire_method_caches(CACHED_METHODS)
end end
# Expires the caches of a specific set of methods
def expire_method_caches(methods)
methods.each do |key|
cache.expire(key)
ivar = cache_instance_variable_name(key)
remove_instance_variable(ivar) if instance_variable_defined?(ivar)
end
end
def expire_avatar_cache def expire_avatar_cache
expire_method_caches(%i(avatar)) expire_method_caches(%i(avatar))
end end
...@@ -924,49 +898,6 @@ class Repository ...@@ -924,49 +898,6 @@ class Repository
end end
end end
# Caches the supplied block both in a cache and in an instance variable.
#
# The cache key and instance variable are named the same way as the value of
# the `key` argument.
#
# This method will return `nil` if the corresponding instance variable is also
# set to `nil`. This ensures we don't keep yielding the block when it returns
# `nil`.
#
# key - The name of the key to cache the data in.
# fallback - A value to fall back to in the event of a Git error.
def cache_method_output(key, fallback: nil, memoize_only: false, &block)
ivar = cache_instance_variable_name(key)
if instance_variable_defined?(ivar)
instance_variable_get(ivar)
else
# If the repository doesn't exist and a fallback was specified we return
# that value inmediately. This saves us Rugged/gRPC invocations.
return fallback unless fallback.nil? || exists?
begin
value =
if memoize_only
yield
else
cache.fetch(key, &block)
end
instance_variable_set(ivar, value)
rescue Gitlab::Git::Repository::NoRepository
# Even if the above `#exists?` check passes these errors might still
# occur (for example because of a non-existing HEAD). We want to
# gracefully handle this and not cache anything
fallback
end
end
end
def cache_instance_variable_name(key)
:"@#{key.to_s.tr('?!', '')}"
end
def file_on_head(type) def file_on_head(type)
if head = tree(:head) if head = tree(:head)
head.blobs.find do |blob| head.blobs.find do |blob|
...@@ -1021,8 +952,7 @@ class Repository ...@@ -1021,8 +952,7 @@ class Repository
end end
def cache def cache
# TODO: should we use UUIDs here? We could move repositories without clearing this cache @cache ||= Gitlab::RepositoryCache.new(self)
@cache ||= RepositoryCache.new(full_path, @project.id)
end end
def tags_sorted_by_committed_date def tags_sorted_by_committed_date
......
...@@ -17,15 +17,7 @@ module MergeRequests ...@@ -17,15 +17,7 @@ module MergeRequests
return @conflicts_can_be_resolved_in_ui = false unless merge_request.has_complete_diff_refs? return @conflicts_can_be_resolved_in_ui = false unless merge_request.has_complete_diff_refs?
return @conflicts_can_be_resolved_in_ui = false if merge_request.branch_missing? return @conflicts_can_be_resolved_in_ui = false if merge_request.branch_missing?
begin @conflicts_can_be_resolved_in_ui = conflicts.can_be_resolved_in_ui?
# Try to parse each conflict. If the MR's mergeable status hasn't been
# updated, ensure that we don't say there are conflicts to resolve
# when there are no conflict files.
conflicts.files.each(&:lines)
@conflicts_can_be_resolved_in_ui = conflicts.files.length > 0
rescue Gitlab::Git::CommandError, Gitlab::Git::Conflict::Parser::UnresolvableError, Gitlab::Git::Conflict::Resolver::ConflictSideMissing
@conflicts_can_be_resolved_in_ui = false
end
end end
def conflicts def conflicts
......
---
title: Cache MergeRequests can_be_resolved_in_ui? git operations
merge_request: 17589
author:
type: performance
module Gitlab module Gitlab
module Conflict module Conflict
class FileCollection class FileCollection
include Gitlab::RepositoryCacheAdapter
attr_reader :merge_request, :resolver attr_reader :merge_request, :resolver
def initialize(merge_request) def initialize(merge_request)
our_commit = merge_request.source_branch_head.raw our_commit = merge_request.source_branch_head.raw
their_commit = merge_request.target_branch_head.raw their_commit = merge_request.target_branch_head.raw
target_repo = merge_request.target_project.repository.raw @target_repo = merge_request.target_project.repository
@source_repo = merge_request.source_project.repository.raw @source_repo = merge_request.source_project.repository.raw
@resolver = Gitlab::Git::Conflict::Resolver.new(target_repo, our_commit.id, their_commit.id) @our_commit_id = our_commit.id
@their_commit_id = their_commit.id
@resolver = Gitlab::Git::Conflict::Resolver.new(@target_repo.raw, @our_commit_id, @their_commit_id)
@merge_request = merge_request @merge_request = merge_request
end end
...@@ -30,6 +34,17 @@ module Gitlab ...@@ -30,6 +34,17 @@ module Gitlab
end end
end end
def can_be_resolved_in_ui?
# Try to parse each conflict. If the MR's mergeable status hasn't been
# updated, ensure that we don't say there are conflicts to resolve
# when there are no conflict files.
files.each(&:lines)
files.any?
rescue Gitlab::Git::CommandError, Gitlab::Git::Conflict::Parser::UnresolvableError, Gitlab::Git::Conflict::Resolver::ConflictSideMissing
false
end
cache_method :can_be_resolved_in_ui?
def file_for_path(old_path, new_path) def file_for_path(old_path, new_path)
files.find { |file| file.their_path == old_path && file.our_path == new_path } files.find { |file| file.their_path == old_path && file.our_path == new_path }
end end
...@@ -56,6 +71,19 @@ Merge branch '#{merge_request.target_branch}' into '#{merge_request.source_branc ...@@ -56,6 +71,19 @@ Merge branch '#{merge_request.target_branch}' into '#{merge_request.source_branc
#{conflict_filenames.join("\n")} #{conflict_filenames.join("\n")}
EOM EOM
end end
private
def cache
@cache ||= begin
# Use the commit ids as a namespace so if the MR branches get
# updated we instantiate the cache under a different namespace. That
# way don't have to worry about explicitly invalidating the cache
namespace = "#{@our_commit_id}:#{@their_commit_id}"
Gitlab::RepositoryCache.new(@target_repo, extra_namespace: namespace)
end
end
end end
end end
end end
# Interface to the Redis-backed cache store
module Gitlab
class RepositoryCache
attr_reader :repository, :namespace, :backend
def initialize(repository, extra_namespace: nil, backend: Rails.cache)
@repository = repository
@namespace = "#{repository.full_path}:#{repository.project.id}"
@namespace += ":#{extra_namespace}" if extra_namespace
@backend = backend
end
def cache_key(type)
"#{type}:#{namespace}"
end
def expire(key)
backend.delete(cache_key(key))
end
def fetch(key, &block)
backend.fetch(cache_key(key), &block)
end
def exist?(key)
backend.exist?(cache_key(key))
end
def read(key)
backend.read(cache_key(key))
end
end
end
module Gitlab
module RepositoryCacheAdapter
extend ActiveSupport::Concern
class_methods do
# Wraps around the given method and caches its output in Redis and an instance
# variable.
#
# This only works for methods that do not take any arguments.
def cache_method(name, fallback: nil, memoize_only: false)
original = :"_uncached_#{name}"
alias_method(original, name)
define_method(name) do
cache_method_output(name, fallback: fallback, memoize_only: memoize_only) do
__send__(original) # rubocop:disable GitlabSecurity/PublicSend
end
end
end
end
# RepositoryCache to be used. Should be overridden by the including class
def cache
raise NotImplementedError
end
# Caches the supplied block both in a cache and in an instance variable.
#
# The cache key and instance variable are named the same way as the value of
# the `key` argument.
#
# This method will return `nil` if the corresponding instance variable is also
# set to `nil`. This ensures we don't keep yielding the block when it returns
# `nil`.
#
# key - The name of the key to cache the data in.
# fallback - A value to fall back to in the event of a Git error.
def cache_method_output(key, fallback: nil, memoize_only: false, &block)
ivar = cache_instance_variable_name(key)
if instance_variable_defined?(ivar)
instance_variable_get(ivar)
else
# If the repository doesn't exist and a fallback was specified we return
# that value inmediately. This saves us Rugged/gRPC invocations.
return fallback unless fallback.nil? || cache.repository.exists?
begin
value =
if memoize_only
yield
else
cache.fetch(key, &block)
end
instance_variable_set(ivar, value)
rescue Gitlab::Git::Repository::NoRepository
# Even if the above `#exists?` check passes these errors might still
# occur (for example because of a non-existing HEAD). We want to
# gracefully handle this and not cache anything
fallback
end
end
end
# Expires the caches of a specific set of methods
def expire_method_caches(methods)
methods.each do |key|
cache.expire(key)
ivar = cache_instance_variable_name(key)
remove_instance_variable(ivar) if instance_variable_defined?(ivar)
end
end
private
def cache_instance_variable_name(key)
:"@#{key.to_s.tr('?!', '')}"
end
end
end
# Interface to the Redis-backed cache store used by the Repository model
class RepositoryCache
attr_reader :namespace, :backend, :project_id
def initialize(namespace, project_id, backend = Rails.cache)
@namespace = namespace
@backend = backend
@project_id = project_id
end
def cache_key(type)
"#{type}:#{namespace}:#{project_id}"
end
def expire(key)
backend.delete(cache_key(key))
end
def fetch(key, &block)
backend.fetch(cache_key(key), &block)
end
def exist?(key)
backend.exist?(cache_key(key))
end
def read(key)
backend.read(cache_key(key))
end
end
...@@ -10,6 +10,38 @@ describe Gitlab::Conflict::FileCollection do ...@@ -10,6 +10,38 @@ describe Gitlab::Conflict::FileCollection do
end end
end end
describe '#cache' do
it 'specifies a custom namespace with the merge request commit ids' do
our_commit = merge_request.source_branch_head.raw
their_commit = merge_request.target_branch_head.raw
custom_namespace = "#{our_commit.id}:#{their_commit.id}"
expect(file_collection.send(:cache).namespace).to include(custom_namespace)
end
end
describe '#can_be_resolved_in_ui?' do
it 'returns true if conflicts for this collection can be resolved in the UI' do
expect(file_collection.can_be_resolved_in_ui?).to be true
end
it "returns false if conflicts for this collection can't be resolved in the UI" do
expect(file_collection).to receive(:files).and_raise(Gitlab::Git::Conflict::Resolver::ConflictSideMissing)
expect(file_collection.can_be_resolved_in_ui?).to be false
end
it 'caches the result' do
expect(file_collection).to receive(:files).twice.and_call_original
expect(file_collection.can_be_resolved_in_ui?).to be true
expect(file_collection).not_to receive(:files)
expect(file_collection.can_be_resolved_in_ui?).to be true
end
end
describe '#default_commit_message' do describe '#default_commit_message' do
it 'matches the format of the git CLI commit message' do it 'matches the format of the git CLI commit message' do
expect(file_collection.default_commit_message).to eq(<<EOM.chomp) expect(file_collection.default_commit_message).to eq(<<EOM.chomp)
......
require 'spec_helper'
describe Gitlab::RepositoryCacheAdapter do
let(:project) { create(:project, :repository) }
let(:repository) { project.repository }
let(:cache) { repository.send(:cache) }
describe '#cache_method_output', :use_clean_rails_memory_store_caching do
let(:fallback) { 10 }
context 'with a non-existing repository' do
let(:project) { create(:project) } # No repository
subject do
repository.cache_method_output(:cats, fallback: fallback) do
repository.cats_call_stub
end
end
it 'returns the fallback value' do
expect(subject).to eq(fallback)
end
it 'avoids calling the original method' do
expect(repository).not_to receive(:cats_call_stub)
subject
end
end
context 'with a method throwing a non-existing-repository error' do
subject do
repository.cache_method_output(:cats, fallback: fallback) do
raise Gitlab::Git::Repository::NoRepository
end
end
it 'returns the fallback value' do
expect(subject).to eq(fallback)
end
it 'does not cache the data' do
subject
expect(repository.instance_variable_defined?(:@cats)).to eq(false)
expect(cache.exist?(:cats)).to eq(false)
end
end
context 'with an existing repository' do
it 'caches the output' do
object = double
expect(object).to receive(:number).once.and_return(10)
2.times do
val = repository.cache_method_output(:cats) { object.number }
expect(val).to eq(10)
end
expect(repository.send(:cache).exist?(:cats)).to eq(true)
expect(repository.instance_variable_get(:@cats)).to eq(10)
end
end
end
describe '#expire_method_caches' do
it 'expires the caches of the given methods' do
expect(cache).to receive(:expire).with(:readme)
expect(cache).to receive(:expire).with(:gitignore)
repository.expire_method_caches(%i(readme gitignore))
end
end
end
require 'spec_helper' require 'spec_helper'
describe RepositoryCache do describe Gitlab::RepositoryCache do
let(:project) { create(:project) }
let(:backend) { double('backend').as_null_object } let(:backend) { double('backend').as_null_object }
let(:cache) { described_class.new('example', project.id, backend) } let(:project) { create(:project) }
let(:repository) { project.repository }
let(:namespace) { "#{repository.full_path}:#{project.id}" }
let(:cache) { described_class.new(repository, backend: backend) }
describe '#cache_key' do describe '#cache_key' do
subject { cache.cache_key(:foo) }
it 'includes the namespace' do it 'includes the namespace' do
expect(cache.cache_key(:foo)).to eq "foo:example:#{project.id}" expect(subject).to eq "foo:#{namespace}"
end
context 'with a given namespace' do
let(:extra_namespace) { 'my:data' }
let(:cache) do
described_class.new(repository, extra_namespace: extra_namespace,
backend: backend)
end
it 'includes the full namespace' do
expect(subject).to eq "foo:#{namespace}:#{extra_namespace}"
end
end end
end end
describe '#expire' do describe '#expire' do
it 'expires the given key from the cache' do it 'expires the given key from the cache' do
cache.expire(:foo) cache.expire(:foo)
expect(backend).to have_received(:delete).with("foo:example:#{project.id}") expect(backend).to have_received(:delete).with("foo:#{namespace}")
end end
end end
describe '#fetch' do describe '#fetch' do
it 'fetches the given key from the cache' do it 'fetches the given key from the cache' do
cache.fetch(:bar) cache.fetch(:bar)
expect(backend).to have_received(:fetch).with("bar:example:#{project.id}") expect(backend).to have_received(:fetch).with("bar:#{namespace}")
end end
it 'accepts a block' do it 'accepts a block' do
p = -> {} p = -> {}
cache.fetch(:baz, &p) cache.fetch(:baz, &p)
expect(backend).to have_received(:fetch).with("baz:example:#{project.id}", &p) expect(backend).to have_received(:fetch).with("baz:#{namespace}", &p)
end end
end end
end end
...@@ -2169,15 +2169,6 @@ describe Repository do ...@@ -2169,15 +2169,6 @@ describe Repository do
end end
end end
describe '#expire_method_caches' do
it 'expires the caches of the given methods' do
expect_any_instance_of(RepositoryCache).to receive(:expire).with(:readme)
expect_any_instance_of(RepositoryCache).to receive(:expire).with(:gitignore)
repository.expire_method_caches(%i(readme gitignore))
end
end
describe '#expire_all_method_caches' do describe '#expire_all_method_caches' do
it 'expires the caches of all methods' do it 'expires the caches of all methods' do
expect(repository).to receive(:expire_method_caches) expect(repository).to receive(:expire_method_caches)
...@@ -2323,66 +2314,6 @@ describe Repository do ...@@ -2323,66 +2314,6 @@ describe Repository do
end end
end end
describe '#cache_method_output', :use_clean_rails_memory_store_caching do
let(:fallback) { 10 }
context 'with a non-existing repository' do
let(:project) { create(:project) } # No repository
subject do
repository.cache_method_output(:cats, fallback: fallback) do
repository.cats_call_stub
end
end
it 'returns the fallback value' do
expect(subject).to eq(fallback)
end
it 'avoids calling the original method' do
expect(repository).not_to receive(:cats_call_stub)
subject
end
end
context 'with a method throwing a non-existing-repository error' do
subject do
repository.cache_method_output(:cats, fallback: fallback) do
raise Gitlab::Git::Repository::NoRepository
end
end
it 'returns the fallback value' do
expect(subject).to eq(fallback)
end
it 'does not cache the data' do
subject
expect(repository.instance_variable_defined?(:@cats)).to eq(false)
expect(repository.send(:cache).exist?(:cats)).to eq(false)
end
end
context 'with an existing repository' do
it 'caches the output' do
object = double
expect(object).to receive(:number).once.and_return(10)
2.times do
val = repository.cache_method_output(:cats) { object.number }
expect(val).to eq(10)
end
expect(repository.send(:cache).exist?(:cats)).to eq(true)
expect(repository.instance_variable_get(:@cats)).to eq(10)
end
end
end
describe '#refresh_method_caches' do describe '#refresh_method_caches' do
it 'refreshes the caches of the given types' do it 'refreshes the caches of the given types' do
expect(repository).to receive(:expire_method_caches) expect(repository).to receive(:expire_method_caches)
......
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