Commit 2f2b74f1 authored by pbair's avatar pbair

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.
parent 80efecbd
...@@ -4,7 +4,7 @@ class BackgroundMigrationWorker # rubocop:disable Scalability/IdempotentWorker ...@@ -4,7 +4,7 @@ class BackgroundMigrationWorker # rubocop:disable Scalability/IdempotentWorker
include BackgroundMigration::SingleDatabaseWorker include BackgroundMigration::SingleDatabaseWorker
def self.tracking_database def self.tracking_database
@tracking_database ||= Gitlab::Database::MAIN_DATABASE_NAME.to_sym @tracking_database ||= Gitlab::BackgroundMigration::DEFAULT_TRACKING_DATABASE
end end
def self.unhealthy_metric_name def self.unhealthy_metric_name
......
...@@ -2,11 +2,13 @@ ...@@ -2,11 +2,13 @@
module Gitlab module Gitlab
module BackgroundMigration module BackgroundMigration
DEFAULT_TRACKING_DATABASE = Gitlab::Database::MAIN_DATABASE_NAME
def self.coordinator_for_database(database) def self.coordinator_for_database(database)
JobCoordinator.for_database(database) JobCoordinator.for_tracking_database(database)
end end
def self.queue(database: :main) def self.queue(database: DEFAULT_TRACKING_DATABASE)
coordinator_for_database(database).queue coordinator_for_database(database).queue
end end
...@@ -22,7 +24,7 @@ module Gitlab ...@@ -22,7 +24,7 @@ module Gitlab
# steal_class - The name of the class for which to steal jobs. # 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. # retry_dead_jobs - Flag to control whether jobs in Sidekiq::RetrySet or Sidekiq::DeadSet are retried.
# database - tracking database this migration executes against # 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) coordinator_for_database(database).steal(steal_class, retry_dead_jobs: retry_dead_jobs, &block)
end end
...@@ -35,15 +37,15 @@ module Gitlab ...@@ -35,15 +37,15 @@ module Gitlab
# arguments - The arguments to pass to the background migration's "perform" # arguments - The arguments to pass to the background migration's "perform"
# method. # method.
# database - tracking database this migration executes against # 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) coordinator_for_database(database).perform(class_name, arguments)
end 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 coordinator_for_database(database).exists?(migration_class, additional_queues) # rubocop:disable CodeReuse/ActiveRecord
end end
def self.remaining(database: :main) def self.remaining(database: DEFAULT_TRACKING_DATABASE)
coordinator_for_database(database).remaining coordinator_for_database(database).remaining
end end
end end
......
...@@ -8,24 +8,33 @@ module Gitlab ...@@ -8,24 +8,33 @@ module Gitlab
# convention of how the queues and worker classes are setup for each database. # convention of how the queues and worker classes are setup for each database.
# #
# Also provides a database connection to the correct tracking database. # Also provides a database connection to the correct tracking database.
class JobCoordinator class JobCoordinator # rubocop:disable Metrics/ClassLength
VALID_DATABASES = %i[main].freeze class << self
WORKER_CLASS_NAME = 'BackgroundMigrationWorker' def worker_classes
@worker_classes ||= [
def self.for_database(database) BackgroundMigrationWorker
database = database.to_sym ].freeze
end
unless VALID_DATABASES.include?(database) def worker_for_tracking_database
raise ArgumentError, "database must be one of [#{VALID_DATABASES.join(', ')}], got '#{database}'" @worker_for_tracking_database ||= worker_classes
.index_by(&:tracking_database)
.with_indifferent_access
.freeze
end end
namespace = database.to_s.capitalize unless database == :main def for_tracking_database(tracking_database)
namespaced_worker_class = [namespace, WORKER_CLASS_NAME].compact.join('::') 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 end
attr_reader :database, :worker_class attr_reader :worker_class
def queue def queue
@queue ||= worker_class.sidekiq_options['queue'] @queue ||= worker_class.sidekiq_options['queue']
...@@ -118,15 +127,14 @@ module Gitlab ...@@ -118,15 +127,14 @@ module Gitlab
private private
def initialize(database, worker_class) def initialize(worker_class)
@database = database
@worker_class = worker_class @worker_class = worker_class
end end
def connection def connection
@connection ||= Gitlab::Database @connection ||= Gitlab::Database
.database_base_models .database_base_models
.fetch(database, Gitlab::Database::PRIMARY_DATABASE_NAME) .fetch(worker_class.tracking_database, Gitlab::Database::PRIMARY_DATABASE_NAME)
.connection .connection
end end
end end
......
...@@ -3,32 +3,22 @@ ...@@ -3,32 +3,22 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Gitlab::BackgroundMigration::JobCoordinator do RSpec.describe Gitlab::BackgroundMigration::JobCoordinator do
let(:database) { :main }
let(:worker_class) { BackgroundMigrationWorker } 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 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) expect(coordinator.worker_class).to eq(worker_class)
end 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 context 'when an invalid value is given' do
it 'raises an error' do it 'raises an error' do
expect do expect do
described_class.for_database('notvalid') described_class.for_tracking_database('notvalid')
end.to raise_error(ArgumentError, "database must be one of [main], got 'notvalid'") end.to raise_error(ArgumentError, /tracking_database must be one of/)
end end
end end
end end
......
...@@ -3,11 +3,12 @@ ...@@ -3,11 +3,12 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Gitlab::BackgroundMigration do 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 before do
allow(described_class).to receive(:coordinator_for_database) allow(described_class).to receive(:coordinator_for_database)
.with(:main) .with(default_tracking_database)
.and_return(coordinator) .and_return(coordinator)
end end
......
...@@ -356,7 +356,7 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do ...@@ -356,7 +356,7 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do
end end
describe '#finalized_background_migration' do 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_name) { 'TestJob' }
let!(:job_class) { Class.new } let!(:job_class) { Class.new }
...@@ -378,7 +378,7 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do ...@@ -378,7 +378,7 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do
job_class.define_method(:perform, job_perform_method) job_class.define_method(:perform, job_perform_method)
allow(Gitlab::BackgroundMigration).to receive(:coordinator_for_database) 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) expect(job_coordinator).to receive(:migration_class_for)
.with(job_class_name).at_least(:once) { job_class } .with(job_class_name).at_least(:once) { job_class }
......
...@@ -59,11 +59,11 @@ RSpec.shared_examples 'it runs background migration jobs' do |tracking_database, ...@@ -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) allow(Postgresql::ReplicationSlot).to receive(:lag_too_great?).and_return(false)
end 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| expect_next_instance_of(Gitlab::BackgroundMigration::JobCoordinator) do |coordinator|
allow(coordinator).to receive(:with_shared_connection).and_yield 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]) expect(coordinator).to receive(:perform).with('Foo', [10, 20])
end end
......
...@@ -3,5 +3,5 @@ ...@@ -3,5 +3,5 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe BackgroundMigrationWorker, :clean_gitlab_redis_shared_state do 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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment