Commit 6fc54496 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'project-cache-worker-scheduling' into 'master'

Don't schedule ProjectCacheWorker unless needed

## What does this MR do?

This MR changes `ProjectCacheWorker.perform_async` so scheduling only takes place when needed. See the commits for more details.

See merge request !7099
parents 279ffe7b 3b4af59a
......@@ -28,6 +28,7 @@ Please view this file on the master branch, on stable branches it's out of date.
- Fix unauthorized users dragging on issue boards
- Better handle when no users were selected for adding to group or project. (Linus Thiel)
- Only show register tab if signup enabled.
- Only schedule ProjectCacheWorker jobs when needed
## 8.13.0 (2016-10-22)
- Removes extra line for empty issue description. (!7045)
......
......@@ -9,6 +9,18 @@ class ProjectCacheWorker
LEASE_TIMEOUT = 15.minutes.to_i
def self.lease_for(project_id)
Gitlab::ExclusiveLease.
new("project_cache_worker:#{project_id}", timeout: LEASE_TIMEOUT)
end
# Overwrite Sidekiq's implementation so we only schedule when actually needed.
def self.perform_async(project_id)
# If a lease for this project is still being held there's no point in
# scheduling a new job.
super unless lease_for(project_id).exists?
end
def perform(project_id)
if try_obtain_lease_for(project_id)
Rails.logger.
......@@ -37,8 +49,6 @@ class ProjectCacheWorker
end
def try_obtain_lease_for(project_id)
Gitlab::ExclusiveLease.
new("project_cache_worker:#{project_id}", timeout: LEASE_TIMEOUT).
try_obtain
self.class.lease_for(project_id).try_obtain
end
end
......@@ -27,7 +27,7 @@ module Gitlab
# on begin/ensure blocks to cancel a lease, because the 'ensure' does
# not always run. Think of 'kill -9' from the Unicorn master for
# instance.
#
#
# If you find that leases are getting in your way, ask yourself: would
# it be enough to lower the lease timeout? Another thing that might be
# appropriate is to only use a lease for bulk/automated operations, and
......@@ -48,6 +48,13 @@ module Gitlab
end
end
# Returns true if the key for this lease is set.
def exists?
Gitlab::Redis.with do |redis|
redis.exists(redis_key)
end
end
# No #cancel method. See comments above!
private
......
require 'spec_helper'
describe Gitlab::ExclusiveLease do
it 'cannot obtain twice before the lease has expired' do
lease = Gitlab::ExclusiveLease.new(unique_key, timeout: 3600)
expect(lease.try_obtain).to eq(true)
expect(lease.try_obtain).to eq(false)
end
describe Gitlab::ExclusiveLease, type: :redis do
let(:unique_key) { SecureRandom.hex(10) }
describe '#try_obtain' do
it 'cannot obtain twice before the lease has expired' do
lease = Gitlab::ExclusiveLease.new(unique_key, timeout: 3600)
expect(lease.try_obtain).to eq(true)
expect(lease.try_obtain).to eq(false)
end
it 'can obtain after the lease has expired' do
timeout = 1
lease = Gitlab::ExclusiveLease.new(unique_key, timeout: timeout)
lease.try_obtain # start the lease
sleep(2 * timeout) # lease should have expired now
expect(lease.try_obtain).to eq(true)
it 'can obtain after the lease has expired' do
timeout = 1
lease = Gitlab::ExclusiveLease.new(unique_key, timeout: timeout)
lease.try_obtain # start the lease
sleep(2 * timeout) # lease should have expired now
expect(lease.try_obtain).to eq(true)
end
end
def unique_key
SecureRandom.hex(10)
describe '#exists?' do
it 'returns true for an existing lease' do
lease = Gitlab::ExclusiveLease.new(unique_key, timeout: 3600)
lease.try_obtain
expect(lease.exists?).to eq(true)
end
it 'returns false for a lease that does not exist' do
lease = Gitlab::ExclusiveLease.new(unique_key, timeout: 3600)
expect(lease.exists?).to eq(false)
end
end
end
......@@ -50,6 +50,12 @@ RSpec.configure do |config|
example.run
Rails.cache = caching_store
end
config.around(:each, :redis) do |example|
Gitlab::Redis.with(&:flushall)
example.run
Gitlab::Redis.with(&:flushall)
end
end
FactoryGirl::SyntaxRunner.class_eval do
......
......@@ -5,6 +5,26 @@ describe ProjectCacheWorker do
subject { described_class.new }
describe '.perform_async' do
it 'schedules the job when no lease exists' do
allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:exists?).
and_return(false)
expect_any_instance_of(described_class).to receive(:perform)
described_class.perform_async(project.id)
end
it 'does not schedule the job when a lease exists' do
allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:exists?).
and_return(true)
expect_any_instance_of(described_class).not_to receive(:perform)
described_class.perform_async(project.id)
end
end
describe '#perform' do
context 'when an exclusive lease can be obtained' do
before do
......
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