Merge branch '343047-remove-hardcoded-database-name' into 'master'

Use constant to refer to main database name

See merge request gitlab-org/gitlab!74557

app/workers/background_migration_worker.rb

@@ -4,7 +4,7 @@ class BackgroundMigrationWorker # rubocop:disable Scalability/IdempotentWorker
   include BackgroundMigration::SingleDatabaseWorker
 
   def self.tracking_database
-    @tracking_database ||= Gitlab::Database::MAIN_DATABASE_NAME.to_sym
+    @tracking_database ||= Gitlab::BackgroundMigration::DEFAULT_TRACKING_DATABASE
   end
 
   def self.unhealthy_metric_name

lib/gitlab/background_migration.rb

@@ -2,11 +2,13 @@
 
 module Gitlab
   module BackgroundMigration
+    DEFAULT_TRACKING_DATABASE = Gitlab::Database::MAIN_DATABASE_NAME
+
     def self.coordinator_for_database(database)
-      JobCoordinator.for_database(database)
+      JobCoordinator.for_tracking_database(database)
     end
 
-    def self.queue(database: :main)
+    def self.queue(database: DEFAULT_TRACKING_DATABASE)
       coordinator_for_database(database).queue
     end
 
@@ -22,7 +24,7 @@ module Gitlab
     # steal_class - The name of the class for which to steal jobs.
     # retry_dead_jobs - Flag to control whether jobs in Sidekiq::RetrySet or Sidekiq::DeadSet are retried.
     # database - tracking database this migration executes against
-    def self.steal(steal_class, retry_dead_jobs: false, database: :main, &block)
+    def self.steal(steal_class, retry_dead_jobs: false, database: DEFAULT_TRACKING_DATABASE, &block)
       coordinator_for_database(database).steal(steal_class, retry_dead_jobs: retry_dead_jobs, &block)
     end
 
@@ -35,15 +37,15 @@ module Gitlab
     # arguments - The arguments to pass to the background migration's "perform"
     #             method.
     # database - tracking database this migration executes against
-    def self.perform(class_name, arguments, database: :main)
+    def self.perform(class_name, arguments, database: DEFAULT_TRACKING_DATABASE)
       coordinator_for_database(database).perform(class_name, arguments)
     end
 
-    def self.exists?(migration_class, additional_queues = [], database: :main)
+    def self.exists?(migration_class, additional_queues = [], database: DEFAULT_TRACKING_DATABASE)
       coordinator_for_database(database).exists?(migration_class, additional_queues) # rubocop:disable CodeReuse/ActiveRecord
     end
 
-    def self.remaining(database: :main)
+    def self.remaining(database: DEFAULT_TRACKING_DATABASE)
       coordinator_for_database(database).remaining
     end
   end

lib/gitlab/background_migration/job_coordinator.rb

@@ -8,24 +8,33 @@ module Gitlab
     # convention of how the queues and worker classes are setup for each database.
     #
     # Also provides a database connection to the correct tracking database.
-    class JobCoordinator
-      VALID_DATABASES = %i[main].freeze
-      WORKER_CLASS_NAME = 'BackgroundMigrationWorker'
-
-      def self.for_database(database)
-        database = database.to_sym
+    class JobCoordinator # rubocop:disable Metrics/ClassLength
+      class << self
+        def worker_classes
+          @worker_classes ||= [
+            BackgroundMigrationWorker
+          ].freeze
+        end
 
-        unless VALID_DATABASES.include?(database)
-          raise ArgumentError, "database must be one of [#{VALID_DATABASES.join(', ')}], got '#{database}'"
+        def worker_for_tracking_database
+          @worker_for_tracking_database ||= worker_classes
+            .index_by(&:tracking_database)
+            .with_indifferent_access
+            .freeze
         end
 
-        namespace = database.to_s.capitalize unless database == :main
-        namespaced_worker_class = [namespace, WORKER_CLASS_NAME].compact.join('::')
+        def for_tracking_database(tracking_database)
+          worker_class = worker_for_tracking_database[tracking_database]
 
-        new(database, "::#{namespaced_worker_class}".constantize)
+          if worker_class.nil?
+            raise ArgumentError, "tracking_database must be one of [#{worker_for_tracking_database.keys.join(', ')}]"
+          end
+
+          new(worker_class)
+        end
       end
 
-      attr_reader :database, :worker_class
+      attr_reader :worker_class
 
       def queue
         @queue ||= worker_class.sidekiq_options['queue']
@@ -118,15 +127,14 @@ module Gitlab
 
       private
 
-      def initialize(database, worker_class)
-        @database = database
+      def initialize(worker_class)
         @worker_class = worker_class
       end
 
       def connection
         @connection ||= Gitlab::Database
           .database_base_models
-          .fetch(database, Gitlab::Database::PRIMARY_DATABASE_NAME)
+          .fetch(worker_class.tracking_database, Gitlab::Database::PRIMARY_DATABASE_NAME)
           .connection
       end
     end

spec/lib/gitlab/background_migration/job_coordinator_spec.rb

@@ -3,32 +3,22 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::BackgroundMigration::JobCoordinator do
-  let(:database) { :main }
   let(:worker_class) { BackgroundMigrationWorker }
-  let(:coordinator) { described_class.new(database, worker_class) }
+  let(:tracking_database) { worker_class.tracking_database }
+  let(:coordinator) { described_class.new(worker_class) }
 
-  describe '.for_database' do
+  describe '.for_tracking_database' do
     it 'returns an executor with the correct worker class and database' do
-      coordinator = described_class.for_database(database)
+      coordinator = described_class.for_tracking_database(tracking_database)
 
-      expect(coordinator.database).to eq(database)
       expect(coordinator.worker_class).to eq(worker_class)
     end
 
-    context 'when passed in as a string' do
-      it 'retruns an executor with the correct worker class and database' do
-        coordinator = described_class.for_database(database.to_s)
-
-        expect(coordinator.database).to eq(database)
-        expect(coordinator.worker_class).to eq(worker_class)
-      end
-    end
-
     context 'when an invalid value is given' do
       it 'raises an error' do
         expect do
-          described_class.for_database('notvalid')
-        end.to raise_error(ArgumentError, "database must be one of [main], got 'notvalid'")
+          described_class.for_tracking_database('notvalid')
+        end.to raise_error(ArgumentError, /tracking_database must be one of/)
       end
     end
   end

spec/lib/gitlab/background_migration_spec.rb

@@ -3,11 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::BackgroundMigration do
-  let(:coordinator) { described_class::JobCoordinator.for_database(:main) }
+  let(:default_tracking_database) { described_class::DEFAULT_TRACKING_DATABASE }
+  let(:coordinator) { described_class::JobCoordinator.for_tracking_database(default_tracking_database) }
 
   before do
     allow(described_class).to receive(:coordinator_for_database)
-      .with(:main)
+      .with(default_tracking_database)
       .and_return(coordinator)
   end
 

spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb

@@ -356,7 +356,7 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do
   end
 
   describe '#finalized_background_migration' do
-    let(:job_coordinator) { Gitlab::BackgroundMigration::JobCoordinator.new(:main, BackgroundMigrationWorker) }
+    let(:job_coordinator) { Gitlab::BackgroundMigration::JobCoordinator.new(BackgroundMigrationWorker) }
 
     let!(:job_class_name) { 'TestJob' }
     let!(:job_class) { Class.new }
@@ -378,7 +378,7 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do
       job_class.define_method(:perform, job_perform_method)
 
       allow(Gitlab::BackgroundMigration).to receive(:coordinator_for_database)
-        .with(:main).and_return(job_coordinator)
+        .with('main').and_return(job_coordinator)
 
       expect(job_coordinator).to receive(:migration_class_for)
         .with(job_class_name).at_least(:once) { job_class }

spec/support/shared_examples/workers/background_migration_worker_shared_examples.rb

@@ -59,11 +59,11 @@ RSpec.shared_examples 'it runs background migration jobs' do |tracking_database,
       allow(Postgresql::ReplicationSlot).to receive(:lag_too_great?).and_return(false)
     end
 
-    it 'performs jobs using the coordinator for the correct database' do
+    it 'performs jobs using the coordinator for the worker' do
       expect_next_instance_of(Gitlab::BackgroundMigration::JobCoordinator) do |coordinator|
         allow(coordinator).to receive(:with_shared_connection).and_yield
 
-        expect(coordinator.database).to eq(tracking_database)
+        expect(coordinator.worker_class).to eq(described_class)
         expect(coordinator).to receive(:perform).with('Foo', [10, 20])
       end
 

spec/workers/background_migration_worker_spec.rb

@@ -3,5 +3,5 @@
 require 'spec_helper'
 
 RSpec.describe BackgroundMigrationWorker, :clean_gitlab_redis_shared_state do
-  it_behaves_like 'it runs background migration jobs', :main, :background_migration_database_health_reschedules
+  it_behaves_like 'it runs background migration jobs', 'main', :background_migration_database_health_reschedules
 end