From 2f2b74f103cde629dc244e3f253cecbbbbc74b9d Mon Sep 17 00:00:00 2001
From: pbair <pbair@gitlab.com>
Date: Mon, 15 Nov 2021 16:55:47 -0500
Subject: [PATCH] Use constant to refer to main database name
Update background migration logic to use a constant rather than
hardcoding the name of the name of the main database.
---
app/workers/background_migration_worker.rb | 2 +-
lib/gitlab/background_migration.rb | 14 ++++---
.../background_migration/job_coordinator.rb | 38 +++++++++++--------
.../job_coordinator_spec.rb | 22 +++--------
spec/lib/gitlab/background_migration_spec.rb | 5 ++-
.../background_migration_helpers_spec.rb | 4 +-
...ground_migration_worker_shared_examples.rb | 4 +-
.../background_migration_worker_spec.rb | 2 +-
8 files changed, 46 insertions(+), 45 deletions(-)
diff --git a/app/workers/background_migration_worker.rb b/app/workers/background_migration_worker.rb
index 6489aad3173e6b..dea0d467ecada0 100644
--- a/app/workers/background_migration_worker.rb
+++ b/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
diff --git a/lib/gitlab/background_migration.rb b/lib/gitlab/background_migration.rb
index 22b4b685f81ee5..ed58508182a7f5 100644
--- a/lib/gitlab/background_migration.rb
+++ b/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 @@ def self.queue(database: :main)
# 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 @@ def self.steal(steal_class, retry_dead_jobs: false, database: :main, &block)
# 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
diff --git a/lib/gitlab/background_migration/job_coordinator.rb b/lib/gitlab/background_migration/job_coordinator.rb
index 1c8819eaa6297f..cfbe7167677132 100644
--- a/lib/gitlab/background_migration/job_coordinator.rb
+++ b/lib/gitlab/background_migration/job_coordinator.rb
@@ -8,24 +8,33 @@ module BackgroundMigration
# 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 @@ def enqueued_job?(queues, migration_class)
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
diff --git a/spec/lib/gitlab/background_migration/job_coordinator_spec.rb b/spec/lib/gitlab/background_migration/job_coordinator_spec.rb
index a0543ca9958f3c..7a524d1489a659 100644
--- a/spec/lib/gitlab/background_migration/job_coordinator_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/background_migration_spec.rb b/spec/lib/gitlab/background_migration_spec.rb
index 777dc8112a715b..8dd7f6892a6d63 100644
--- a/spec/lib/gitlab/background_migration_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb b/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb
index d83e501aabdd84..99c7d70724c281 100644
--- a/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb
+++ b/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb
@@ -356,7 +356,7 @@
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 @@
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 }
diff --git a/spec/support/shared_examples/workers/background_migration_worker_shared_examples.rb b/spec/support/shared_examples/workers/background_migration_worker_shared_examples.rb
index ffde2e11c1d177..0d3e158d3583d4 100644
--- a/spec/support/shared_examples/workers/background_migration_worker_shared_examples.rb
+++ b/spec/support/shared_examples/workers/background_migration_worker_shared_examples.rb
@@ -59,11 +59,11 @@
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
diff --git a/spec/workers/background_migration_worker_spec.rb b/spec/workers/background_migration_worker_spec.rb
index 06513861c0e107..4297e55ca6c196 100644
--- a/spec/workers/background_migration_worker_spec.rb
+++ b/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
--
GitLab