Merge branch 'remove-job-deduplication-feature-flags' into 'master'

Remove implicit job deduplication feature flags

See merge request gitlab-org/gitlab!48031

app/workers/concerns/reenqueuer.rb

@@ -52,7 +52,11 @@ module Reenqueuer
   private
 
   def reenqueue(*args)
-    self.class.perform_async(*args) if yield
+    result = yield
+
+    self.class.perform_async(*args) if result
+
+    result
   end
 
   # Override as needed

doc/development/sidekiq_style_guide.md

@@ -251,26 +251,6 @@ module AuthorizedProjectUpdate
 end
 ```
 
-#### Troubleshooting
-
-If the automatic deduplication were to cause issues in certain
-queues. This can be temporarily disabled by enabling a feature flag
-named `disable_<queue name>_deduplication`. For example to disable
-deduplication for the `AuthorizedProjectsWorker`, we would enable the
-feature flag `disable_authorized_projects_deduplication`.
-
-From ChatOps:
-
-```shell
-/chatops run feature set disable_authorized_projects_deduplication true
-```
-
-From the rails console:
-
-```ruby
-Feature.enable!(:disable_authorized_projects_deduplication)
-```
-
 ## Limited capacity worker
 
 It is possible to limit the number of concurrent running jobs for a worker class

lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job.rb

@@ -70,10 +70,6 @@ module Gitlab
           jid != existing_jid
         end
 
-        def droppable?
-          idempotent? && ::Feature.disabled?("disable_#{queue_name}_deduplication", type: :ops)
-        end
-
         def scheduled_at
           job['at']
         end
@@ -85,6 +81,13 @@ module Gitlab
           worker_klass.get_deduplication_options
         end
 
+        def idempotent?
+          return false unless worker_klass
+          return false unless worker_klass.respond_to?(:idempotent?)
+
+          worker_klass.idempotent?
+        end
+
         private
 
         attr_reader :queue_name, :job
@@ -128,13 +131,6 @@ module Gitlab
         def idempotency_string
           "#{worker_class_name}:#{arguments.join('-')}"
         end
-
-        def idempotent?
-          return false unless worker_klass
-          return false unless worker_klass.respond_to?(:idempotent?)
-
-          worker_klass.idempotent?
-        end
       end
     end
   end

lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/deduplicates_when_scheduling.rb

@@ -13,7 +13,7 @@ module Gitlab
             if deduplicatable_job? && check! && duplicate_job.duplicate?
               job['duplicate-of'] = duplicate_job.existing_jid
 
-              if duplicate_job.droppable?
+              if duplicate_job.idempotent?
                 Gitlab::SidekiqLogging::DeduplicationLogger.instance.log(
                   job, "dropped #{strategy_name}", duplicate_job.options)
                 return false

spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job_spec.rb

@@ -131,31 +131,6 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi
     end
   end
 
-  describe '#droppable?' do
-    where(:idempotent, :prevent_deduplication) do
-      # [true, false].repeated_permutation(2)
-      [[true, true],
-       [true, false],
-       [false, true],
-       [false, false]]
-    end
-
-    with_them do
-      before do
-        allow(AuthorizedProjectsWorker).to receive(:idempotent?).and_return(idempotent)
-        stub_feature_flags("disable_#{queue}_deduplication" => prevent_deduplication)
-      end
-
-      it 'is droppable when all conditions are met' do
-        if idempotent && !prevent_deduplication
-          expect(duplicate_job).to be_droppable
-        else
-          expect(duplicate_job).not_to be_droppable
-        end
-      end
-    end
-  end
-
   describe '#scheduled_at' do
     let(:scheduled_at) { 42 }
     let(:job) do
@@ -181,6 +156,46 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi
     end
   end
 
+  describe '#idempotent?' do
+    context 'when worker class does not exist' do
+      let(:job) { { 'class' => '' } }
+
+      it 'returns false' do
+        expect(duplicate_job).not_to be_idempotent
+      end
+    end
+
+    context 'when worker class does not respond to #idempotent?' do
+      before do
+        stub_const('AuthorizedProjectsWorker', Class.new)
+      end
+
+      it 'returns false' do
+        expect(duplicate_job).not_to be_idempotent
+      end
+    end
+
+    context 'when worker class is not idempotent' do
+      before do
+        allow(AuthorizedProjectsWorker).to receive(:idempotent?).and_return(false)
+      end
+
+      it 'returns false' do
+        expect(duplicate_job).not_to be_idempotent
+      end
+    end
+
+    context 'when worker class is idempotent' do
+      before do
+        allow(AuthorizedProjectsWorker).to receive(:idempotent?).and_return(true)
+      end
+
+      it 'returns true' do
+        expect(duplicate_job).to be_idempotent
+      end
+    end
+  end
+
   def set_idempotency_key(key, value = '1')
     Sidekiq.redis { |r| r.set(key, value) }
   end

spec/support/shared_examples/lib/gitlab/sidekiq_middleware/strategy_shared_examples.rb

@@ -38,7 +38,7 @@ RSpec.shared_examples 'deduplicating jobs when scheduling' do |strategy_name|
       it 'adds the jid of the existing job to the job hash' do
         allow(fake_duplicate_job).to receive(:scheduled?).and_return(false)
         allow(fake_duplicate_job).to receive(:check!).and_return('the jid')
-        allow(fake_duplicate_job).to receive(:droppable?).and_return(true)
+        allow(fake_duplicate_job).to receive(:idempotent?).and_return(true)
         allow(fake_duplicate_job).to receive(:options).and_return({})
         job_hash = {}
 
@@ -62,7 +62,7 @@ RSpec.shared_examples 'deduplicating jobs when scheduling' do |strategy_name|
               receive(:check!)
                 .with(Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob::DUPLICATE_KEY_TTL)
                 .and_return('the jid'))
-            allow(fake_duplicate_job).to receive(:droppable?).and_return(true)
+            allow(fake_duplicate_job).to receive(:idempotent?).and_return(true)
             job_hash = {}
 
             expect(fake_duplicate_job).to receive(:duplicate?).and_return(true)
@@ -82,7 +82,7 @@ RSpec.shared_examples 'deduplicating jobs when scheduling' do |strategy_name|
               allow(fake_duplicate_job).to receive(:options).and_return({ including_scheduled: true })
               allow(fake_duplicate_job).to(
                 receive(:check!).with(time_diff.to_i).and_return('the jid'))
-              allow(fake_duplicate_job).to receive(:droppable?).and_return(true)
+              allow(fake_duplicate_job).to receive(:idempotent?).and_return(true)
               job_hash = {}
 
               expect(fake_duplicate_job).to receive(:duplicate?).and_return(true)
@@ -104,13 +104,13 @@ RSpec.shared_examples 'deduplicating jobs when scheduling' do |strategy_name|
         allow(fake_duplicate_job).to receive(:duplicate?).and_return(true)
         allow(fake_duplicate_job).to receive(:options).and_return({})
         allow(fake_duplicate_job).to receive(:existing_jid).and_return('the jid')
-        allow(fake_duplicate_job).to receive(:droppable?).and_return(true)
+        allow(fake_duplicate_job).to receive(:idempotent?).and_return(true)
       end
 
       it 'drops the job' do
         schedule_result = nil
 
-        expect(fake_duplicate_job).to receive(:droppable?).and_return(true)
+        expect(fake_duplicate_job).to receive(:idempotent?).and_return(true)
 
         expect { |b| schedule_result = strategy.schedule({}, &b) }.not_to yield_control
         expect(schedule_result).to be(false)

spec/workers/concerns/reenqueuer_spec.rb

@@ -79,6 +79,10 @@ RSpec.describe Reenqueuer do
 
         job.perform
       end
+
+      it 'returns the original value from #perform' do
+        expect(job.perform).to eq(true)
+      end
     end
 
     context 'when #perform returns falsey' do
@@ -87,6 +91,10 @@ RSpec.describe Reenqueuer do
 
         job.perform
       end
+
+      it 'returns the original value from #perform' do
+        expect(job.perform).to eq(false)
+      end
     end
   end
 end