From 5f9c84584efd5a7cbe19ada49fdccefbd5f54aea Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 7 Jul 2017 14:49:05 +0200 Subject: Added EachBatch for iterating tables in batches This module provides a class method called `each_batch` that can be used to iterate tables in batches in a more efficient way compared to Rails' `in_batches` method. This commit also includes a RuboCop cop to blacklist the use of `in_batches` in favour of this new method. --- spec/models/concerns/each_batch_spec.rb | 47 +++++++++++++++++++++++++++++++++ spec/rubocop/cop/in_batches_spec.rb | 19 +++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 spec/models/concerns/each_batch_spec.rb create mode 100644 spec/rubocop/cop/in_batches_spec.rb (limited to 'spec') diff --git a/spec/models/concerns/each_batch_spec.rb b/spec/models/concerns/each_batch_spec.rb new file mode 100644 index 00000000000..10d8c59eea4 --- /dev/null +++ b/spec/models/concerns/each_batch_spec.rb @@ -0,0 +1,47 @@ +require 'spec_helper' + +describe EachBatch do + describe '.each_batch' do + let(:model) do + Class.new(ActiveRecord::Base) do + include EachBatch + + self.table_name = 'users' + end + end + + before do + 5.times { create(:user, updated_at: 1.day.ago) } + end + + it 'yields an ActiveRecord::Relation when a block is given' do + model.each_batch do |relation| + expect(relation).to be_a_kind_of(ActiveRecord::Relation) + end + end + + it 'accepts a custom batch size' do + amount = 0 + + model.each_batch(of: 1) { amount += 1 } + + expect(amount).to eq(5) + end + + it 'does not include ORDER BYs in the yielded relations' do + model.each_batch do |relation| + expect(relation.to_sql).not_to include('ORDER BY') + end + end + + it 'allows updating of the yielded relations' do + time = Time.now + + model.each_batch do |relation| + relation.update_all(updated_at: time) + end + + expect(model.where(updated_at: time).count).to eq(5) + end + end +end diff --git a/spec/rubocop/cop/in_batches_spec.rb b/spec/rubocop/cop/in_batches_spec.rb new file mode 100644 index 00000000000..072481984c6 --- /dev/null +++ b/spec/rubocop/cop/in_batches_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../rubocop/cop/in_batches' + +describe RuboCop::Cop::InBatches do + include CopHelper + + subject(:cop) { described_class.new } + + it 'registers an offense when in_batches is used' do + inspect_source(cop, 'foo.in_batches do; end') + + aggregate_failures do + expect(cop.offenses.size).to eq(1) + expect(cop.offenses.map(&:line)).to eq([1]) + end + end +end -- cgit v1.2.1 From 16ae7b7a49314b2525f3f32c07dd8a4891fa74e1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 28 Jun 2017 11:29:04 +0200 Subject: Add initial stage_id background migration files --- ...igrate_stage_id_reference_in_background_spec.rb | 26 ++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 spec/migrations/migrate_stage_id_reference_in_background_spec.rb (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb new file mode 100644 index 00000000000..f86ef834afa --- /dev/null +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20170628080858_migrate_stage_id_reference_in_background') + +describe MigrateStageIdReferenceInBackground, :migration, :redis do + let(:jobs) { table(:ci_builds) } + let(:stages) { table(:ci_stages) } + let(:pipelines) { table(:ci_pipelines) } + let(:projects) { table(:projects) } + + before do + projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1') + pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') + + jobs.create!(id: 1, commit_id: 1, project_id: 123, stage_idx: 2, stage: 'build') + jobs.create!(id: 2, commit_id: 1, project_id: 123, stage_idx: 2, stage: 'build') + jobs.create!(id: 3, commit_id: 1, project_id: 123, stage_idx: 1, stage: 'test') + jobs.create!(id: 4, commit_id: 1, project_id: 123, stage_idx: 3, stage: 'deploy') + + stages.create(id: 101, pipeline_id: 1, project_id: 123, name: 'test') + stages.create(id: 102, pipeline_id: 1, project_id: 123, name: 'build') + stages.create(id: 103, pipeline_id: 1, project_id: 123, name: 'deploy') + end + + it 'schedules background migrations' do + end +end -- cgit v1.2.1 From b7d672328db358690d043aae8b5fc24c358a52ab Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 28 Jun 2017 12:01:52 +0200 Subject: Add initial build stage_id ref background migration --- spec/migrations/migrate_stage_id_reference_in_background_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index f86ef834afa..ea3a18802d9 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -22,5 +22,10 @@ describe MigrateStageIdReferenceInBackground, :migration, :redis do end it 'schedules background migrations' do + expect(jobs.where(stage_id: nil)).to be_present + + migrate! + + expect(jobs.where(stage_id: nil)).to be_empty end end -- cgit v1.2.1 From 9a32ca409757b4b25520ea9957cdd4c8c97c0e95 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 28 Jun 2017 12:21:25 +0200 Subject: Find builds that require a migration in batches --- spec/migrations/migrate_stage_id_reference_in_background_spec.rb | 2 ++ 1 file changed, 2 insertions(+) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index ea3a18802d9..d515eb42b9d 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -8,6 +8,8 @@ describe MigrateStageIdReferenceInBackground, :migration, :redis do let(:projects) { table(:projects) } before do + stub_const('MigrateStageIdReferenceInBackground::BATCH_SIZE', 1) + projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1') pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') -- cgit v1.2.1 From 07693b43745e31e47466a9f1f5b73de894323a5d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 28 Jun 2017 15:24:53 +0200 Subject: Add specs for delayed stage_id background migrations --- ...igrate_stage_id_reference_in_background_spec.rb | 51 ++++++++++++++++++++-- 1 file changed, 47 insertions(+), 4 deletions(-) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index d515eb42b9d..8b497656377 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -1,7 +1,37 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20170628080858_migrate_stage_id_reference_in_background') -describe MigrateStageIdReferenceInBackground, :migration, :redis do +RSpec::Matchers.define :have_migrated do |*expected| + match do |migration| + BackgroundMigrationWorker.jobs.any? do |job| + job['enqueued_at'].present? && job['args'] == [migration, expected] + end + end + + failure_message do |migration| + <<-EOS + Background migration `#{migration}` with args `#{expected.inspect}` + not migrated! + EOS + end +end + +RSpec::Matchers.define :have_scheduled_migration do |time, *expected| + match do |migration| + BackgroundMigrationWorker.jobs.any? do |job| + job['args'] == [migration, expected] && job['at'] >= time + end + end + + failure_message do |migration| + <<-EOS + Background migration `#{migration}` with args `#{expected.inspect}` + not scheduled! + EOS + end +end + +describe MigrateStageIdReferenceInBackground, :migration do let(:jobs) { table(:ci_builds) } let(:stages) { table(:ci_stages) } let(:pipelines) { table(:ci_pipelines) } @@ -23,11 +53,24 @@ describe MigrateStageIdReferenceInBackground, :migration, :redis do stages.create(id: 103, pipeline_id: 1, project_id: 123, name: 'deploy') end + it 'correctly schedules background migrations' do + Sidekiq::Testing.fake! do + migrate! + + expect(described_class::MIGRATION).to have_migrated(1) + expect(described_class::MIGRATION).to have_migrated(2) + expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 3) + expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 4) + end + end + it 'schedules background migrations' do - expect(jobs.where(stage_id: nil)).to be_present + Sidekiq::Testing.inline! do + expect(jobs.where(stage_id: nil)).to be_present - migrate! + migrate! - expect(jobs.where(stage_id: nil)).to be_empty + expect(jobs.where(stage_id: nil)).to be_empty + end end end -- cgit v1.2.1 From 945cdf326edcafdcd7a1d2aeaef611d888a4828e Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 11:41:19 +0200 Subject: Make it possible to schedule bg migrations in bulk --- spec/support/sidekiq.rb | 8 +++++- spec/workers/background_migration_worker_spec.rb | 33 +++++++++++++++++++++++- 2 files changed, 39 insertions(+), 2 deletions(-) (limited to 'spec') diff --git a/spec/support/sidekiq.rb b/spec/support/sidekiq.rb index 575d3451150..f3819ed2353 100644 --- a/spec/support/sidekiq.rb +++ b/spec/support/sidekiq.rb @@ -1,5 +1,11 @@ -require 'sidekiq/testing/inline' +require 'sidekiq/testing' Sidekiq::Testing.server_middleware do |chain| chain.add Gitlab::SidekiqStatus::ServerMiddleware end + +RSpec.configure do |config| + config.after(:each, :sidekiq) do + Sidekiq::Worker.clear_all + end +end diff --git a/spec/workers/background_migration_worker_spec.rb b/spec/workers/background_migration_worker_spec.rb index 85939429feb..4f6e3474634 100644 --- a/spec/workers/background_migration_worker_spec.rb +++ b/spec/workers/background_migration_worker_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe BackgroundMigrationWorker do +describe BackgroundMigrationWorker, :sidekiq do describe '.perform' do it 'performs a background migration' do expect(Gitlab::BackgroundMigration) @@ -10,4 +10,35 @@ describe BackgroundMigrationWorker do described_class.new.perform('Foo', [10, 20]) end end + + describe '.perform_bulk' do + it 'enqueues background migrations in bulk' do + Sidekiq::Testing.fake! do + described_class.perform_bulk([['Foo', [1]], ['Foo', [2]]]) + + expect(described_class.jobs.count).to eq 2 + expect(described_class.jobs).to all(include('enqueued_at')) + end + end + end + + describe '.perform_bulk_in' do + context 'when delay is valid' do + it 'correctly schedules background migrations' do + Sidekiq::Testing.fake! do + described_class.perform_bulk_in(1.minute, [['Foo', [1]], ['Foo', [2]]]) + + expect(described_class.jobs.count).to eq 2 + expect(described_class.jobs).to all(include('at')) + end + end + end + + context 'when delay is invalid' do + it 'raises an ArgumentError exception' do + expect { described_class.perform_bulk_in(-60, [['Foo']]) } + .to raise_error(ArgumentError) + end + end + end end -- cgit v1.2.1 From c2efb8acc609eaec799b536412d1f66311aa1cf7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 12:10:29 +0200 Subject: Use ActiveRecord 5 batching to schedule bg migration --- ...igrate_stage_id_reference_in_background_spec.rb | 55 ++++++++++------------ 1 file changed, 26 insertions(+), 29 deletions(-) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index 8b497656377..d3645ec0395 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -1,44 +1,39 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20170628080858_migrate_stage_id_reference_in_background') -RSpec::Matchers.define :have_migrated do |*expected| - match do |migration| - BackgroundMigrationWorker.jobs.any? do |job| - job['enqueued_at'].present? && job['args'] == [migration, expected] +describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do + matcher :have_migrated do |*expected| + match do |migration| + BackgroundMigrationWorker.jobs.any? do |job| + job['enqueued_at'].present? && job['args'] == [migration, expected] + end end - end - failure_message do |migration| - <<-EOS - Background migration `#{migration}` with args `#{expected.inspect}` - not migrated! - EOS + failure_message do |migration| + "Migration `#{migration}` with args `#{expected.inspect}` not executed!" + end end -end -RSpec::Matchers.define :have_scheduled_migration do |time, *expected| - match do |migration| - BackgroundMigrationWorker.jobs.any? do |job| - job['args'] == [migration, expected] && job['at'] >= time + matcher :have_scheduled_migration do |delay, *expected| + match do |migration| + BackgroundMigrationWorker.jobs.any? do |job| + job['args'] == [migration, expected] && + job['at'].to_f == (delay.to_f + Time.now.to_f) + end end - end - failure_message do |migration| - <<-EOS - Background migration `#{migration}` with args `#{expected.inspect}` - not scheduled! - EOS + failure_message do |migration| + "Migration `#{migration}` with args `#{expected.inspect}` not scheduled!" + end end -end -describe MigrateStageIdReferenceInBackground, :migration do let(:jobs) { table(:ci_builds) } let(:stages) { table(:ci_stages) } let(:pipelines) { table(:ci_pipelines) } let(:projects) { table(:projects) } before do - stub_const('MigrateStageIdReferenceInBackground::BATCH_SIZE', 1) + stub_const('MigrateStageIdReferenceInBackground::BATCH_SIZE', 2) projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1') pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') @@ -55,12 +50,14 @@ describe MigrateStageIdReferenceInBackground, :migration do it 'correctly schedules background migrations' do Sidekiq::Testing.fake! do - migrate! + Timecop.freeze do + migrate! - expect(described_class::MIGRATION).to have_migrated(1) - expect(described_class::MIGRATION).to have_migrated(2) - expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 3) - expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 4) + expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 1) + expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 2) + expect(described_class::MIGRATION).to have_scheduled_migration(10.minutes, 3) + expect(described_class::MIGRATION).to have_scheduled_migration(10.minutes, 4) + end end end -- cgit v1.2.1 From 00ca74810814a9f35a14755f1b91169096640dba Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 12:14:41 +0200 Subject: Remove unused background migrations matcher --- .../migrate_stage_id_reference_in_background_spec.rb | 12 ------------ 1 file changed, 12 deletions(-) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index d3645ec0395..3eeca2e9659 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -2,18 +2,6 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20170628080858_migrate_stage_id_reference_in_background') describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do - matcher :have_migrated do |*expected| - match do |migration| - BackgroundMigrationWorker.jobs.any? do |job| - job['enqueued_at'].present? && job['args'] == [migration, expected] - end - end - - failure_message do |migration| - "Migration `#{migration}` with args `#{expected.inspect}` not executed!" - end - end - matcher :have_scheduled_migration do |delay, *expected| match do |migration| BackgroundMigrationWorker.jobs.any? do |job| -- cgit v1.2.1 From 825521539d26b6434fa8e15b07051c29bccba1c9 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 12:26:37 +0200 Subject: Improve specs for background stage_id ref migration --- .../migrate_stage_id_reference_in_background_spec.rb | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index 3eeca2e9659..046d9b351a8 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20170628080858_migrate_stage_id_reference_in_background') describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do - matcher :have_scheduled_migration do |delay, *expected| + matcher :be_scheduled_migration do |delay, *expected| match do |migration| BackgroundMigrationWorker.jobs.any? do |job| job['args'] == [migration, expected] && @@ -24,16 +24,22 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do stub_const('MigrateStageIdReferenceInBackground::BATCH_SIZE', 2) projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1') + projects.create!(id: 345, name: 'gitlab2', path: 'gitlab2') + pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') + pipelines.create!(id: 2, project_id: 345, ref: 'feature', sha: 'cdf43c3c') jobs.create!(id: 1, commit_id: 1, project_id: 123, stage_idx: 2, stage: 'build') jobs.create!(id: 2, commit_id: 1, project_id: 123, stage_idx: 2, stage: 'build') jobs.create!(id: 3, commit_id: 1, project_id: 123, stage_idx: 1, stage: 'test') jobs.create!(id: 4, commit_id: 1, project_id: 123, stage_idx: 3, stage: 'deploy') + jobs.create!(id: 5, commit_id: 2, project_id: 345, stage_idx: 1, stage: 'test') stages.create(id: 101, pipeline_id: 1, project_id: 123, name: 'test') stages.create(id: 102, pipeline_id: 1, project_id: 123, name: 'build') stages.create(id: 103, pipeline_id: 1, project_id: 123, name: 'deploy') + + jobs.create!(id: 6, commit_id: 2, project_id: 345, stage_id: 101, stage_idx: 1, stage: 'test') end it 'correctly schedules background migrations' do @@ -41,10 +47,11 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do Timecop.freeze do migrate! - expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 1) - expect(described_class::MIGRATION).to have_scheduled_migration(5.minutes, 2) - expect(described_class::MIGRATION).to have_scheduled_migration(10.minutes, 3) - expect(described_class::MIGRATION).to have_scheduled_migration(10.minutes, 4) + expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 1) + expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 2) + expect(described_class::MIGRATION).to be_scheduled_migration(10.minutes, 3) + expect(described_class::MIGRATION).to be_scheduled_migration(10.minutes, 4) + expect(BackgroundMigrationWorker.jobs.size).to eq 5 end end end @@ -55,7 +62,7 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do migrate! - expect(jobs.where(stage_id: nil)).to be_empty + expect(jobs.where(stage_id: nil)).to be_one end end end -- cgit v1.2.1 From 55f93db876f1bd62bde09d4003a0ef9a02adb04c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 14:31:12 +0200 Subject: Make `inline` a default sidekiq testing processing again --- spec/support/sidekiq.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'spec') diff --git a/spec/support/sidekiq.rb b/spec/support/sidekiq.rb index f3819ed2353..5478fea4e64 100644 --- a/spec/support/sidekiq.rb +++ b/spec/support/sidekiq.rb @@ -1,4 +1,4 @@ -require 'sidekiq/testing' +require 'sidekiq/testing/inline' Sidekiq::Testing.server_middleware do |chain| chain.add Gitlab::SidekiqStatus::ServerMiddleware -- cgit v1.2.1 From 01128b130b32fac3481fb3b386b649cb047b4b1f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 15:22:06 +0200 Subject: Use integers to schedule delayed background migrations --- spec/migrations/migrate_stage_id_reference_in_background_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index 046d9b351a8..1bd2c14b61c 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -6,7 +6,7 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do match do |migration| BackgroundMigrationWorker.jobs.any? do |job| job['args'] == [migration, expected] && - job['at'].to_f == (delay.to_f + Time.now.to_f) + job['at'].to_f == (delay.to_i + Time.now.to_i) end end -- cgit v1.2.1 From d953f1762ea9d4be0e53d5280b9f38224b39e67b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 15:26:47 +0200 Subject: Improve readability of build stage id migration query --- spec/migrations/migrate_stage_id_reference_in_background_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index 1bd2c14b61c..63787d71233 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -58,11 +58,11 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do it 'schedules background migrations' do Sidekiq::Testing.inline! do - expect(jobs.where(stage_id: nil)).to be_present + expect(jobs.where(stage_id: nil).count).to eq 5 migrate! - expect(jobs.where(stage_id: nil)).to be_one + expect(jobs.where(stage_id: nil).count).to eq 1 end end end -- cgit v1.2.1 From 14e45c424fa6b147cb32e2ef268b89ab08c37f1f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 29 Jun 2017 15:36:39 +0200 Subject: Do not compare float with integer in migrations specs --- spec/migrations/migrate_stage_id_reference_in_background_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index 63787d71233..2e5504c849d 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -6,7 +6,7 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do match do |migration| BackgroundMigrationWorker.jobs.any? do |job| job['args'] == [migration, expected] && - job['at'].to_f == (delay.to_i + Time.now.to_i) + job['at'].to_i == (delay.to_i + Time.now.to_i) end end -- cgit v1.2.1 From 24a4199ac5eb517e7a27e458d5d1b4f937480a31 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 3 Jul 2017 13:02:51 +0200 Subject: Reduce a delay between stage_id scheduled migrations --- spec/migrations/migrate_stage_id_reference_in_background_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index 2e5504c849d..a32a7fceb68 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -47,10 +47,10 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do Timecop.freeze do migrate! - expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 1) - expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes, 2) - expect(described_class::MIGRATION).to be_scheduled_migration(10.minutes, 3) - expect(described_class::MIGRATION).to be_scheduled_migration(10.minutes, 4) + expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 1) + expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 2) + expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 3) + expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 4) expect(BackgroundMigrationWorker.jobs.size).to eq 5 end end -- cgit v1.2.1 From e036a3743069b7d398117c6742bd6c21239e879a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 6 Jul 2017 15:31:54 +0200 Subject: Add walk_table_in_batches and refactor migration helpers --- spec/lib/gitlab/database/migration_helpers_spec.rb | 53 ++++++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 4259be3f522..65af2e1cb52 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -2,9 +2,7 @@ require 'spec_helper' describe Gitlab::Database::MigrationHelpers, lib: true do let(:model) do - ActiveRecord::Migration.new.extend( - Gitlab::Database::MigrationHelpers - ) + ActiveRecord::Migration.new.extend(described_class) end before do @@ -264,7 +262,7 @@ describe Gitlab::Database::MigrationHelpers, lib: true do describe '#update_column_in_batches' do context 'when running outside of a transaction' do before do - expect(model).to receive(:transaction_open?).and_return(false) + expect(model).to receive(:transaction_open?).twice.and_return(false) create_list(:empty_project, 5) end @@ -313,6 +311,53 @@ describe Gitlab::Database::MigrationHelpers, lib: true do end end + describe '#walk_table_in_batches' do + context 'when running outside of a transaction' do + before do + expect(model).to receive(:transaction_open?).and_return(false) + + create_list(:empty_project, 6) + end + + it 'yields for each batch' do + expect { |b| model.walk_table_in_batches(:projects, of: 2, &b) } + .to yield_control.exactly(3).times + end + + it 'yields successive ranges' do + expect { |b| model.walk_table_in_batches(:projects, of: 2, &b) } + .to yield_successive_args([1, Integer, Integer], + [2, Integer, Integer], + [3, Integer, 0]) + end + + context 'when a scope is provided' do + it 'limits the scope of the statement provided inside the block' do + first_id = Project.first.id + scope = ->(table, query) { query.where(table[:id].eq(first_id)) } + + model.walk_table_in_batches(:projects, scope: scope) do + Arel::UpdateManager.new(ActiveRecord::Base) + .table(Arel::Table.new(:projects)) + .set([[Arel::Table.new(:projects)[:archived], true]]) + end + + expect(Project.where(archived: true).count).to eq(1) + end + end + end + + context 'when running inside the transaction' do + it 'raises RuntimeError' do + expect(model).to receive(:transaction_open?).and_return(true) + + expect do + model.update_column_in_batches(:projects, :star_count, Arel.sql('1+1')) + end.to raise_error(RuntimeError) + end + end + end + describe '#add_column_with_default' do context 'outside of a transaction' do context 'when a column limit is not set' do -- cgit v1.2.1 From 2917f4868a19ab17a0217703c7b842261f8b9e46 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 6 Jul 2017 22:15:12 +0200 Subject: Extract `execute_in_batches` migrations helper --- spec/lib/gitlab/database/migration_helpers_spec.rb | 43 ++++++++++++++++++---- 1 file changed, 36 insertions(+), 7 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 65af2e1cb52..f4a66b7e2a2 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -262,7 +262,8 @@ describe Gitlab::Database::MigrationHelpers, lib: true do describe '#update_column_in_batches' do context 'when running outside of a transaction' do before do - expect(model).to receive(:transaction_open?).twice.and_return(false) + expect(model).to receive(:transaction_open?) + .at_least(:once).and_return(false) create_list(:empty_project, 5) end @@ -336,10 +337,39 @@ describe Gitlab::Database::MigrationHelpers, lib: true do first_id = Project.first.id scope = ->(table, query) { query.where(table[:id].eq(first_id)) } - model.walk_table_in_batches(:projects, scope: scope) do + expect { |b| model.walk_table_in_batches(:projects, of: 1, scope: scope, &b) } + .to yield_control.exactly(:once) + end + end + end + + context 'when running inside the transaction' do + it 'raises RuntimeError' do + expect(model).to receive(:transaction_open?).and_return(true) + + expect { model.walk_table_in_batches(:projects, of: 2) } + .to raise_error(RuntimeError) + end + end + end + + describe '#execute_in_batches' do + context 'when running outside of a transaction' do + before do + expect(model).to receive(:transaction_open?) + .at_least(:once).and_return(false) + + create_list(:empty_project, 6) + end + + context 'when a scope is provided' do + it 'limits the scope of the statement provided inside the block' do + first_id = Project.first.id + scope = ->(table, query) { query.where(table[:id].eq(first_id)) } + + model.execute_in_batches(:projects, scope: scope) do |table| Arel::UpdateManager.new(ActiveRecord::Base) - .table(Arel::Table.new(:projects)) - .set([[Arel::Table.new(:projects)[:archived], true]]) + .table(table).set([[table[:archived], true]]) end expect(Project.where(archived: true).count).to eq(1) @@ -351,9 +381,8 @@ describe Gitlab::Database::MigrationHelpers, lib: true do it 'raises RuntimeError' do expect(model).to receive(:transaction_open?).and_return(true) - expect do - model.update_column_in_batches(:projects, :star_count, Arel.sql('1+1')) - end.to raise_error(RuntimeError) + expect { model.execute_in_batches(:projects)} + .to raise_error(RuntimeError) end end end -- cgit v1.2.1 From fb89ba24825853ca29b804a4a08f7c83210f09db Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 7 Jul 2017 09:35:28 +0200 Subject: Schedule stage_id background migration in ranges --- .../migrate_stage_id_reference_in_background_spec.rb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index a32a7fceb68..f3dde8b59c0 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -21,7 +21,7 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do let(:projects) { table(:projects) } before do - stub_const('MigrateStageIdReferenceInBackground::BATCH_SIZE', 2) + stub_const("#{described_class.name}::BATCH_SIZE", 2) projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1') projects.create!(id: 345, name: 'gitlab2', path: 'gitlab2') @@ -47,11 +47,10 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do Timecop.freeze do migrate! - expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 1) - expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 2) - expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 3) - expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 4) - expect(BackgroundMigrationWorker.jobs.size).to eq 5 + expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 1, 3) + expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 3, 5) + expect(described_class::MIGRATION).to be_scheduled_migration(6.minutes, 5, 0) + expect(BackgroundMigrationWorker.jobs.size).to eq 3 end end end -- cgit v1.2.1 From af0eeefc324e96d79c85b337ae9e441947a9f729 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 7 Jul 2017 15:05:27 +0200 Subject: Revert recent changes in migration helpers --- spec/lib/gitlab/database/migration_helpers_spec.rb | 82 ++-------------------- 1 file changed, 4 insertions(+), 78 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index f4a66b7e2a2..4259be3f522 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -2,7 +2,9 @@ require 'spec_helper' describe Gitlab::Database::MigrationHelpers, lib: true do let(:model) do - ActiveRecord::Migration.new.extend(described_class) + ActiveRecord::Migration.new.extend( + Gitlab::Database::MigrationHelpers + ) end before do @@ -262,8 +264,7 @@ describe Gitlab::Database::MigrationHelpers, lib: true do describe '#update_column_in_batches' do context 'when running outside of a transaction' do before do - expect(model).to receive(:transaction_open?) - .at_least(:once).and_return(false) + expect(model).to receive(:transaction_open?).and_return(false) create_list(:empty_project, 5) end @@ -312,81 +313,6 @@ describe Gitlab::Database::MigrationHelpers, lib: true do end end - describe '#walk_table_in_batches' do - context 'when running outside of a transaction' do - before do - expect(model).to receive(:transaction_open?).and_return(false) - - create_list(:empty_project, 6) - end - - it 'yields for each batch' do - expect { |b| model.walk_table_in_batches(:projects, of: 2, &b) } - .to yield_control.exactly(3).times - end - - it 'yields successive ranges' do - expect { |b| model.walk_table_in_batches(:projects, of: 2, &b) } - .to yield_successive_args([1, Integer, Integer], - [2, Integer, Integer], - [3, Integer, 0]) - end - - context 'when a scope is provided' do - it 'limits the scope of the statement provided inside the block' do - first_id = Project.first.id - scope = ->(table, query) { query.where(table[:id].eq(first_id)) } - - expect { |b| model.walk_table_in_batches(:projects, of: 1, scope: scope, &b) } - .to yield_control.exactly(:once) - end - end - end - - context 'when running inside the transaction' do - it 'raises RuntimeError' do - expect(model).to receive(:transaction_open?).and_return(true) - - expect { model.walk_table_in_batches(:projects, of: 2) } - .to raise_error(RuntimeError) - end - end - end - - describe '#execute_in_batches' do - context 'when running outside of a transaction' do - before do - expect(model).to receive(:transaction_open?) - .at_least(:once).and_return(false) - - create_list(:empty_project, 6) - end - - context 'when a scope is provided' do - it 'limits the scope of the statement provided inside the block' do - first_id = Project.first.id - scope = ->(table, query) { query.where(table[:id].eq(first_id)) } - - model.execute_in_batches(:projects, scope: scope) do |table| - Arel::UpdateManager.new(ActiveRecord::Base) - .table(table).set([[table[:archived], true]]) - end - - expect(Project.where(archived: true).count).to eq(1) - end - end - end - - context 'when running inside the transaction' do - it 'raises RuntimeError' do - expect(model).to receive(:transaction_open?).and_return(true) - - expect { model.execute_in_batches(:projects)} - .to raise_error(RuntimeError) - end - end - end - describe '#add_column_with_default' do context 'outside of a transaction' do context 'when a column limit is not set' do -- cgit v1.2.1 From b41b4d2e6a44a04fc3e6fff09cf45f93033ff0ad Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 7 Jul 2017 15:19:43 +0200 Subject: Use new `each_batch` helper instead of custom one In stage_id backgrond migration. --- spec/migrations/migrate_stage_id_reference_in_background_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index f3dde8b59c0..90ad5c39089 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -47,9 +47,9 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do Timecop.freeze do migrate! - expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 1, 3) - expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 3, 5) - expect(described_class::MIGRATION).to be_scheduled_migration(6.minutes, 5, 0) + expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 1, 2) + expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 3, 4) + expect(described_class::MIGRATION).to be_scheduled_migration(6.minutes, 5, 6) expect(BackgroundMigrationWorker.jobs.size).to eq 3 end end -- cgit v1.2.1 From c467451ea6f39f498b458e11b5f8a74c53d3541d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 7 Jul 2017 15:44:47 +0200 Subject: Schedule stage_id bg migrations in batches of 10 --- .../migrate_stage_id_reference_in_background_spec.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index 90ad5c39089..e829a9238f6 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -21,7 +21,8 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do let(:projects) { table(:projects) } before do - stub_const("#{described_class.name}::BATCH_SIZE", 2) + stub_const("#{described_class.name}::BATCH_SIZE", 3) + stub_const("#{described_class.name}::RANGE_SIZE", 2) projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1') projects.create!(id: 345, name: 'gitlab2', path: 'gitlab2') @@ -48,9 +49,10 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do migrate! expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 1, 2) - expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 3, 4) - expect(described_class::MIGRATION).to be_scheduled_migration(6.minutes, 5, 6) - expect(BackgroundMigrationWorker.jobs.size).to eq 3 + expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 3, 3) + expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 4, 5) + expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 6, 6) + expect(BackgroundMigrationWorker.jobs.size).to eq 4 end end end -- cgit v1.2.1 From 320229e12aea3c5f52bcf0fb413bb35138ef7c25 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 7 Jul 2017 15:50:33 +0200 Subject: Do not schedule bg migration when it is not needed --- spec/migrations/migrate_stage_id_reference_in_background_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'spec') diff --git a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb index e829a9238f6..260378adaa7 100644 --- a/spec/migrations/migrate_stage_id_reference_in_background_spec.rb +++ b/spec/migrations/migrate_stage_id_reference_in_background_spec.rb @@ -51,8 +51,7 @@ describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 1, 2) expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 3, 3) expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 4, 5) - expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 6, 6) - expect(BackgroundMigrationWorker.jobs.size).to eq 4 + expect(BackgroundMigrationWorker.jobs.size).to eq 3 end end end -- cgit v1.2.1