Commit 2ce0d063 authored by Yorick Peterse's avatar Yorick Peterse

Smarter flushing of branch statistics caches

Instead of flushing the behind/ahead counts for all branches upon every
push we now only flush the cache of branches that actually need to have
these statistics recalculated. There are now basically 2 scenarios and
their effects:

1. A user pushes a commit to the default branch, this results in the
   cache being flushed for all branches.
2. A user pushes to a non default branch, this results in _only_ the
   cache for that branch being flushed.

The existing code (Repository#expire_cache) remains backwards compatible
with the previous behaviour, the new behaviour is only applied when a
branch name is passed as an argument. This ensures that when for example
a project is deleted the cache for all branches is flushed.
parent 643c6186
...@@ -205,12 +205,6 @@ class Repository ...@@ -205,12 +205,6 @@ class Repository
readme version contribution_guide changelog license) readme version contribution_guide changelog license)
end end
def branch_cache_keys
branches.map do |branch|
:"diverging_commit_counts_#{branch.name}"
end
end
def build_cache def build_cache
cache_keys.each do |key| cache_keys.each do |key|
unless cache.exist?(key) unless cache.exist?(key)
...@@ -235,18 +229,27 @@ class Repository ...@@ -235,18 +229,27 @@ class Repository
@branches = nil @branches = nil
end end
def expire_cache def expire_cache(branch_name = nil)
cache_keys.each do |key| cache_keys.each do |key|
cache.expire(key) cache.expire(key)
end end
expire_branch_cache expire_branch_cache(branch_name)
end end
def expire_branch_cache def expire_branch_cache(branch_name = nil)
# When we push to the root branch we have to flush the cache for all other
# branches as their statistics are based on the commits relative to the
# root branch.
if !branch_name || branch_name == root_ref
branches.each do |branch| branches.each do |branch|
cache.expire(:"diverging_commit_counts_#{branch.name}") cache.expire(:"diverging_commit_counts_#{branch.name}")
end end
# In case a commit is pushed to a non-root branch we only have to flush the
# cache for said branch.
else
cache.expire(:"diverging_commit_counts_#{branch_name}")
end
end end
def expire_root_ref_cache def expire_root_ref_cache
......
...@@ -18,7 +18,9 @@ class GitPushService ...@@ -18,7 +18,9 @@ class GitPushService
def execute(project, user, oldrev, newrev, ref) def execute(project, user, oldrev, newrev, ref)
@project, @user = project, user @project, @user = project, user
project.repository.expire_cache branch_name = Gitlab::Git.ref_name(ref)
project.repository.expire_cache(branch_name)
if push_remove_branch?(ref, newrev) if push_remove_branch?(ref, newrev)
project.repository.expire_has_visible_content_cache project.repository.expire_has_visible_content_cache
...@@ -33,7 +35,6 @@ class GitPushService ...@@ -33,7 +35,6 @@ class GitPushService
@push_commits = project.repository.commits(newrev) @push_commits = project.repository.commits(newrev)
# Ensure HEAD points to the default branch in case it is not master # Ensure HEAD points to the default branch in case it is not master
branch_name = Gitlab::Git.ref_name(ref)
project.change_head(branch_name) project.change_head(branch_name)
# Set protection on the default branch if configured # Set protection on the default branch if configured
......
...@@ -291,6 +291,12 @@ describe Repository, models: true do ...@@ -291,6 +291,12 @@ describe Repository, models: true do
repository.expire_cache repository.expire_cache
end end
it 'expires the caches for a specific branch' do
expect(repository).to receive(:expire_branch_cache).with('master')
repository.expire_cache('master')
end
end end
describe '#expire_root_ref_cache' do describe '#expire_root_ref_cache' do
...@@ -320,4 +326,32 @@ describe Repository, models: true do ...@@ -320,4 +326,32 @@ describe Repository, models: true do
expect(repository.has_visible_content?).to eq(false) expect(repository.has_visible_content?).to eq(false)
end end
end end
describe '#expire_branch_ache' do
# This method is private but we need it for testing purposes. Sadly there's
# no other proper way of testing caching operations.
let(:cache) { repository.send(:cache) }
it 'expires the cache for all branches' do
expect(cache).to receive(:expire).
at_least(repository.branches.length).
times
repository.expire_branch_cache
end
it 'expires the cache for all branches when the root branch is given' do
expect(cache).to receive(:expire).
at_least(repository.branches.length).
times
repository.expire_branch_cache(repository.root_ref)
end
it 'expires the cache for a specific branch' do
expect(cache).to receive(:expire).once
repository.expire_branch_cache('foo')
end
end
end end
...@@ -23,7 +23,7 @@ describe GitPushService, services: true do ...@@ -23,7 +23,7 @@ describe GitPushService, services: true do
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
it 'flushes general cached data' do it 'flushes general cached data' do
expect(project.repository).to receive(:expire_cache) expect(project.repository).to receive(:expire_cache).with('master')
subject subject
end end
...@@ -43,7 +43,7 @@ describe GitPushService, services: true do ...@@ -43,7 +43,7 @@ describe GitPushService, services: true do
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
it 'flushes general cached data' do it 'flushes general cached data' do
expect(project.repository).to receive(:expire_cache) expect(project.repository).to receive(:expire_cache).with('master')
subject subject
end end
...@@ -63,7 +63,7 @@ describe GitPushService, services: true do ...@@ -63,7 +63,7 @@ describe GitPushService, services: true do
end end
it 'flushes general cached data' do it 'flushes general cached data' do
expect(project.repository).to receive(:expire_cache) expect(project.repository).to receive(:expire_cache).with('master')
subject subject
end end
......
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