Commit e41d42d6 authored by Grzegorz Bizon's avatar Grzegorz Bizon
Browse files

Simplify background migrations stealing code

Simply re-raise an exception when it occurs, but guarantee that no
background migration is lost in the process.
parent af41bd41
......@@ -27,9 +27,7 @@ def self.steal(steal_class)
begin
perform(migration_class, migration_args, retries: 3) if job.delete
rescue StandardError
next
rescue Exception
rescue Exception # rubocop:disable Lint/RescueException
BackgroundMigrationWorker # enqueue this migration again
.perform_async(migration_class, migration_args)
......@@ -40,25 +38,15 @@ def self.steal(steal_class)
end
##
# Performs a background migration. In case of `StandardError` being caught
# this will retry a migration up to three times.
# Performs a background migration.
#
# class_name - The name of the background migration class as defined in the
# Gitlab::BackgroundMigration namespace.
#
# arguments - The arguments to pass to the background migration's "perform"
# method.
def self.perform(class_name, arguments, retries: 0)
def self.perform(class_name, arguments)
const_get(class_name).new.perform(*arguments)
rescue StandardError
if retries > 0
Rails.logger.warn("Retrying background migration #{class_name} " \
"with #{arguments}")
retries -= 1
retry
else
raise
end
end
end
end
......@@ -62,32 +62,14 @@
allow(queue[1]).to receive(:delete).and_return(true)
end
context 'when standard error is being raised' do
it 'recovers from an exception and retries the migration' do
expect(migration).to receive(:perform).with(10, 20)
.and_raise(StandardError, 'Migration error')
.exactly(4).times.ordered
expect(migration).to receive(:perform).with(20, 30)
.once.ordered
expect(Rails.logger).to receive(:warn)
.with(/Retrying background migration/).exactly(3).times
described_class.steal('Foo')
end
end
context 'when top level exception is being raised' do
it 'enqueues the migration again and reraises the error' do
allow(migration).to receive(:perform).with(10, 20)
.and_raise(Exception, 'Migration error').once
it 'enqueues the migration again and re-raises the error' do
allow(migration).to receive(:perform).with(10, 20)
.and_raise(Exception, 'Migration error').once
expect(BackgroundMigrationWorker).to receive(:perform_async)
.with('Foo', [10, 20]).once
expect(BackgroundMigrationWorker).to receive(:perform_async)
.with('Foo', [10, 20]).once
expect(Rails.logger).not_to receive(:warn)
expect { described_class.steal('Foo') }
.to raise_error(Exception)
end
expect { described_class.steal('Foo') }.to raise_error(Exception)
end
end
end
......@@ -131,42 +113,10 @@
stub_const("#{described_class.name}::Foo", migration)
end
context 'when retries count is not specified' do
it 'performs a background migration' do
expect(migration).to receive(:perform).with(10, 20).once
described_class.perform('Foo', [10, 20])
end
end
context 'when retries count is zero' do
it 'perform a background migration only once' do
expect(migration).to receive(:perform).with(10, 20)
.and_raise(StandardError).once
it 'performs a background migration' do
expect(migration).to receive(:perform).with(10, 20).once
expect { described_class.perform('Foo', [10, 20], retries: 0) }
.to raise_error(StandardError)
end
end
context 'when retries count is one' do
it 'retries a background migration when needed' do
expect(migration).to receive(:perform).with(10, 20)
.and_raise(StandardError).twice
expect { described_class.perform('Foo', [10, 20], retries: 1) }
.to raise_error(StandardError)
end
end
context 'when retries count is larger than zero' do
it 'retries a background migration when needed' do
expect(migration).to receive(:perform).with(10, 20)
.and_raise(StandardError).exactly(4).times
expect { described_class.perform('Foo', [10, 20], retries: 3) }
.to raise_error(StandardError)
end
described_class.perform('Foo', [10, 20])
end
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