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

Don't schedule ProjectCacheWorker unless needed

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

See merge request !7099
Signed-off-by: Rémy Coutable <remy@rymai.me>

CHANGELOG.md

@@ -21,6 +21,7 @@ Please view this file on the master branch, on stable branches it's out of date.
   - Fix `User#to_reference`. !7088
   - Reduce overhead of `LabelFinder` by avoiding `#presence` call. !7094
   - Fix unauthorized users dragging on issue boards. !7096
+  - Only schedule `ProjectCacheWorker` jobs when needed. !7099
 
 ## 8.13.0 (2016-10-22)
 

app/workers/project_cache_worker.rb

@@ -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

lib/gitlab/exclusive_lease.rb

@@ -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

spec/lib/gitlab/exclusive_lease_spec.rb

 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

spec/spec_helper.rb

@@ -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

spec/workers/project_cache_worker_spec.rb

@@ -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