Merge branch '325519-prevent_retry_for_delayed_data_consistency' into 'master'

Prevent retry for delayed data consistency

See merge request gitlab-org/gitlab!65267

app/workers/concerns/application_worker.rb

@@ -47,11 +47,36 @@ module ApplicationWorker
   end
 
   class_methods do
+    extend ::Gitlab::Utils::Override
+
     def inherited(subclass)
       subclass.set_queue
       subclass.after_set_class_attribute { subclass.set_queue }
     end
 
+    override :validate_worker_attributes!
+    def validate_worker_attributes!
+      super
+
+      # Since the delayed data_consistency will use sidekiq built in retry mechanism, it is required that this mechanism
+      # is not disabled.
+      if retry_disabled? && get_data_consistency == :delayed
+        raise ArgumentError, "Retry support cannot be disabled if data_consistency is set to :delayed"
+      end
+    end
+
+    # Checks if sidekiq retry support is disabled
+    def retry_disabled?
+      get_sidekiq_options['retry'] == 0 || get_sidekiq_options['retry'] == false
+    end
+
+    override :sidekiq_options
+    def sidekiq_options(opts = {})
+      super.tap do
+        validate_worker_attributes!
+      end
+    end
+
     def perform_async(*args)
       # Worker execution for workers with data_consistency set to :delayed or :sticky
       # will be delayed to give replication enough time to complete

ee/app/workers/concerns/elastic/bulk_cron_worker.rb

@@ -6,8 +6,6 @@ module Elastic
 
     included do
       include ApplicationWorker
-
-      sidekiq_options retry: 3
       include Gitlab::ExclusiveLeaseHelpers
       # There is no onward scheduling and this cron handles work from across the
       # application, so there's no useful context to add.

ee/app/workers/elastic_index_bulk_cron_worker.rb

@@ -7,7 +7,7 @@ class ElasticIndexBulkCronWorker # rubocop:disable Scalability/IdempotentWorker
   urgency :throttled
   # Even though this worker is idempotent, until https://gitlab.com/gitlab-org/gitlab/-/issues/325291 is done
   # we can't use it with read-only database replicas
-  data_consistency :delayed
+  data_consistency :sticky
 
   private
 

ee/app/workers/elastic_index_initial_bulk_cron_worker.rb

@@ -7,7 +7,7 @@ class ElasticIndexInitialBulkCronWorker # rubocop:disable Scalability/Idempotent
   urgency :throttled
   # Even though this worker is idempotent, until https://gitlab.com/gitlab-org/gitlab/-/issues/325291 is done
   # we can't use it with read-only database replicas
-  data_consistency :delayed
+  data_consistency :sticky
 
   private
 

ee/spec/workers/elastic_index_bulk_cron_worker_spec.rb

@@ -50,5 +50,5 @@ RSpec.describe ElasticIndexBulkCronWorker do
 
   it_behaves_like 'worker with data consistency',
                   described_class,
-                  data_consistency: :delayed
+                  data_consistency: :sticky
 end

ee/spec/workers/elastic_index_initial_bulk_cron_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ElasticIndexInitialBulkCronWorker do
+  it_behaves_like 'worker with data consistency',
+                  described_class,
+                  data_consistency: :sticky
+end

spec/workers/concerns/application_worker_spec.rb

@@ -176,6 +176,77 @@ RSpec.describe ApplicationWorker do
     end
   end
 
+  describe '.data_consistency' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:data_consistency, :sidekiq_option_retry, :expect_error) do
+      :delayed  | false | true
+      :delayed  | 0     | true
+      :delayed  | 3     | false
+      :delayed  | nil   | false
+      :sticky   | false | false
+      :sticky   | 0     | false
+      :sticky   | 3     | false
+      :sticky   | nil   | false
+      :always   | false | false
+      :always   | 0     | false
+      :always   | 3     | false
+      :always   | nil   | false
+    end
+
+    with_them do
+      before do
+        worker.sidekiq_options retry: sidekiq_option_retry unless sidekiq_option_retry.nil?
+      end
+
+      context "when workers data consistency is #{params['data_consistency']}" do
+        it "#{params['expect_error'] ? '' : 'not to '}raise an exception" do
+          if expect_error
+            expect { worker.data_consistency data_consistency }
+              .to raise_error("Retry support cannot be disabled if data_consistency is set to :delayed")
+          else
+            expect { worker.data_consistency data_consistency }
+              .not_to raise_error
+          end
+        end
+      end
+    end
+  end
+
+  describe '.retry' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:data_consistency, :sidekiq_option_retry, :expect_error) do
+      :delayed  | false | true
+      :delayed  | 0     | true
+      :delayed  | 3     | false
+      :sticky   | false | false
+      :sticky   | 0     | false
+      :sticky   | 3     | false
+      :always   | false | false
+      :always   | 0     | false
+      :always   | 3     | false
+    end
+
+    with_them do
+      before do
+        worker.data_consistency(data_consistency)
+      end
+
+      context "when retry sidekiq option is #{params['sidekiq_option_retry']}" do
+        it "#{params['expect_error'] ? '' : 'not to '}raise an exception" do
+          if expect_error
+            expect { worker.sidekiq_options retry: sidekiq_option_retry }
+              .to raise_error("Retry support cannot be disabled if data_consistency is set to :delayed")
+          else
+            expect { worker.sidekiq_options retry: sidekiq_option_retry }
+              .not_to raise_error
+          end
+        end
+      end
+    end
+  end
+
   describe '.perform_async' do
     shared_examples_for 'worker utilizes load balancing capabilities' do |data_consistency|
       before do