Spread out the work a little

4 files changed, 117 insertions, 10 deletions

diff --git a/db/post_migrate/20170907170235_delete_conflicting_redirect_routes.rb b/db/post_migrate/20170907170235_delete_conflicting_redirect_routes.rb
index 021b52b9ed1..e2e1cddd9e8 100644
--- a/db/post_migrate/20170907170235_delete_conflicting_redirect_routes.rb
+++ b/db/post_migrate/20170907170235_delete_conflicting_redirect_routes.rb
@@ -18,7 +18,7 @@ class DeleteConflictingRedirectRoutes < ActiveRecord::Migration
   def up
     say opening_message
 
-    queue_background_migration_jobs_by_range(Route, MIGRATION)
+    queue_background_migration_jobs_by_range_at_intervals(Route, MIGRATION, 1.minute)
   end
 
   def down
diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb
index 18aefc9558b..2c35da8f1aa 100644
--- a/lib/gitlab/database/migration_helpers.rb
+++ b/lib/gitlab/database/migration_helpers.rb
@@ -657,7 +657,9 @@ into similar problems in the future (e.g. when new tables are created).
         end
       end
 
-      # Queues background migration jobs for an entire table, batched by ID range.
+      # Bulk queues background migration jobs for an entire table, batched by ID range.
+      # "Bulk" meaning many jobs will be pushed at a time for efficiency.
+      # If you need a delay interval per job, then use `queue_background_migration_jobs_by_range_at_intervals`.
       #
       # model_class - The table being iterated over
       # job_class_name - The background migration job class as a string
@@ -670,7 +672,7 @@ into similar problems in the future (e.g. when new tables are created).
       #       self.table_name = 'routes'
       #     end
       #
-      #     queue_background_migration_jobs_by_range(Route, 'ProcessRoutes')
+      #     bulk_queue_background_migration_jobs_by_range(Route, 'ProcessRoutes')
       #
       # Where the model_class includes EachBatch, and the background migration exists:
       #
@@ -679,7 +681,7 @@ into similar problems in the future (e.g. when new tables are created).
       #         # do something
       #       end
       #     end
-      def queue_background_migration_jobs_by_range(model_class, job_class_name, batch_size = BACKGROUND_MIGRATION_BATCH_SIZE)
+      def bulk_queue_background_migration_jobs_by_range(model_class, job_class_name, batch_size: BACKGROUND_MIGRATION_BATCH_SIZE)
         raise "#{model_class} does not have an ID to use for batch ranges" unless model_class.column_names.include?('id')
 
         jobs = []
@@ -701,6 +703,44 @@ into similar problems in the future (e.g. when new tables are created).
 
         BackgroundMigrationWorker.perform_bulk(jobs) unless jobs.empty?
       end
+
+      # Queues background migration jobs for an entire table, batched by ID range.
+      # Each job is scheduled with a `delay_interval` in between.
+      # If you use a small interval, then some jobs may run at the same time.
+      #
+      # model_class - The table being iterated over
+      # job_class_name - The background migration job class as a string
+      # delay_interval - The duration between each job's scheduled time (must respond to `to_f`)
+      # batch_size - The maximum number of rows per job
+      #
+      # Example:
+      #
+      #     class Route < ActiveRecord::Base
+      #       include EachBatch
+      #       self.table_name = 'routes'
+      #     end
+      #
+      #     queue_background_migration_jobs_by_range_at_intervals(Route, 'ProcessRoutes', 1.minute)
+      #
+      # Where the model_class includes EachBatch, and the background migration exists:
+      #
+      #     class Gitlab::BackgroundMigration::ProcessRoutes
+      #       def perform(start_id, end_id)
+      #         # do something
+      #       end
+      #     end
+      def queue_background_migration_jobs_by_range_at_intervals(model_class, job_class_name, delay_interval, batch_size: BACKGROUND_MIGRATION_BATCH_SIZE)
+        raise "#{model_class} does not have an ID to use for batch ranges" unless model_class.column_names.include?('id')
+
+        model_class.each_batch(of: batch_size) do |relation, index|
+          start_id, end_id = relation.pluck('MIN(id), MAX(id)').first
+
+          # `BackgroundMigrationWorker.bulk_perform_in` schedules all jobs for
+          # the same time, which is not helpful in most cases where we wish to
+          # spread the work over time.
+          BackgroundMigrationWorker.perform_in(delay_interval * index, job_class_name, [start_id, end_id])
+        end
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb
index fab77889059..3c8350b3aad 100644
--- a/spec/lib/gitlab/database/migration_helpers_spec.rb
+++ b/spec/lib/gitlab/database/migration_helpers_spec.rb
@@ -915,7 +915,7 @@ describe Gitlab::Database::MigrationHelpers do
     end
   end
 
-  describe '#queue_background_migration_jobs_by_range', :sidekiq do
+  describe '#bulk_queue_background_migration_jobs_by_range', :sidekiq do
     context 'when the model has an ID column' do
       let!(:id1) { create(:user).id }
       let!(:id2) { create(:user).id }
@@ -934,7 +934,7 @@ describe Gitlab::Database::MigrationHelpers do
 
         it 'queues jobs correctly' do
           Sidekiq::Testing.fake! do
-            model.queue_background_migration_jobs_by_range(User, 'FooJob', 2)
+            model.bulk_queue_background_migration_jobs_by_range(User, 'FooJob', batch_size: 2)
 
             expect(BackgroundMigrationWorker.jobs[0]['args']).to eq(['FooJob', [id1, id2]])
             expect(BackgroundMigrationWorker.jobs[1]['args']).to eq(['FooJob', [id3, id3]])
@@ -945,14 +945,14 @@ describe Gitlab::Database::MigrationHelpers do
           expect(BackgroundMigrationWorker).to receive(:perform_bulk).with([['FooJob', [id1, id2]]])
           expect(BackgroundMigrationWorker).to receive(:perform_bulk).with([['FooJob', [id3, id3]]])
 
-          model.queue_background_migration_jobs_by_range(User, 'FooJob', 2)
+          model.bulk_queue_background_migration_jobs_by_range(User, 'FooJob', batch_size: 2)
         end
       end
 
       context 'with not enough rows to bulk queue jobs more than once' do
         it 'queues jobs correctly' do
           Sidekiq::Testing.fake! do
-            model.queue_background_migration_jobs_by_range(User, 'FooJob', 2)
+            model.bulk_queue_background_migration_jobs_by_range(User, 'FooJob', batch_size: 2)
 
             expect(BackgroundMigrationWorker.jobs[0]['args']).to eq(['FooJob', [id1, id2]])
             expect(BackgroundMigrationWorker.jobs[1]['args']).to eq(['FooJob', [id3, id3]])
@@ -963,7 +963,67 @@ describe Gitlab::Database::MigrationHelpers do
           expect(BackgroundMigrationWorker).to receive(:perform_bulk).with([['FooJob', [id1, id2]],
                                                                             ['FooJob', [id3, id3]]])
 
-          model.queue_background_migration_jobs_by_range(User, 'FooJob', 2)
+          model.bulk_queue_background_migration_jobs_by_range(User, 'FooJob', batch_size: 2)
+        end
+      end
+
+      context 'without specifying batch_size' do
+        it 'queues jobs correctly' do
+          Sidekiq::Testing.fake! do
+            model.bulk_queue_background_migration_jobs_by_range(User, 'FooJob')
+
+            expect(BackgroundMigrationWorker.jobs[0]['args']).to eq(['FooJob', [id1, id3]])
+          end
+        end
+      end
+    end
+
+    context "when the model doesn't have an ID column" do
+      it 'raises error (for now)' do
+        expect do
+          model.bulk_queue_background_migration_jobs_by_range(ProjectAuthorization, 'FooJob')
+        end.to raise_error(StandardError, /does not have an ID/)
+      end
+    end
+  end
+
+  describe '#queue_background_migration_jobs_by_range_at_intervals', :sidekiq do
+    context 'when the model has an ID column' do
+      let!(:id1) { create(:user).id }
+      let!(:id2) { create(:user).id }
+      let!(:id3) { create(:user).id }
+
+      around do |example|
+        Timecop.freeze { example.run }
+      end
+
+      before do
+        User.class_eval do
+          include EachBatch
+        end
+      end
+
+      context 'with batch_size option' do
+        it 'queues jobs correctly' do
+          Sidekiq::Testing.fake! do
+            model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.seconds, batch_size: 2)
+
+            expect(BackgroundMigrationWorker.jobs[0]['args']).to eq(['FooJob', [id1, id2]])
+            expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(10.seconds.from_now.to_f)
+            expect(BackgroundMigrationWorker.jobs[1]['args']).to eq(['FooJob', [id3, id3]])
+            expect(BackgroundMigrationWorker.jobs[1]['at']).to eq(20.seconds.from_now.to_f)
+          end
+        end
+      end
+
+      context 'without batch_size option' do
+        it 'queues jobs correctly' do
+          Sidekiq::Testing.fake! do
+            model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.seconds)
+
+            expect(BackgroundMigrationWorker.jobs[0]['args']).to eq(['FooJob', [id1, id3]])
+            expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(10.seconds.from_now.to_f)
+          end
         end
       end
     end
@@ -971,7 +1031,7 @@ describe Gitlab::Database::MigrationHelpers do
     context "when the model doesn't have an ID column" do
       it 'raises error (for now)' do
         expect do
-          model.queue_background_migration_jobs_by_range(ProjectAuthorization, 'FooJob')
+          model.queue_background_migration_jobs_by_range_at_intervals(ProjectAuthorization, 'FooJob', 10.seconds)
         end.to raise_error(StandardError, /does not have an ID/)
       end
     end
diff --git a/spec/migrations/delete_conflicting_redirect_routes_spec.rb b/spec/migrations/delete_conflicting_redirect_routes_spec.rb
index b79d04c1a21..cf872595f21 100644
--- a/spec/migrations/delete_conflicting_redirect_routes_spec.rb
+++ b/spec/migrations/delete_conflicting_redirect_routes_spec.rb
@@ -5,6 +5,10 @@ describe DeleteConflictingRedirectRoutes, :migration, :sidekiq do
   let!(:redirect_routes) { table(:redirect_routes) }
   let!(:routes) { table(:routes) }
 
+  around do |example|
+    Timecop.freeze { example.run }
+  end
+
   before do
     stub_const("Gitlab::Database::MigrationHelpers::BACKGROUND_MIGRATION_BATCH_SIZE", 2)
     stub_const("Gitlab::Database::MigrationHelpers::BACKGROUND_MIGRATION_JOB_BUFFER_SIZE", 2)
@@ -34,8 +38,11 @@ describe DeleteConflictingRedirectRoutes, :migration, :sidekiq do
         migrate!
 
         expect(BackgroundMigrationWorker.jobs[0]['args']).to eq([described_class::MIGRATION, [1, 2]])
+        expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(1.minute.from_now.to_f)
         expect(BackgroundMigrationWorker.jobs[1]['args']).to eq([described_class::MIGRATION, [3, 4]])
+        expect(BackgroundMigrationWorker.jobs[1]['at']).to eq(2.minutes.from_now.to_f)
         expect(BackgroundMigrationWorker.jobs[2]['args']).to eq([described_class::MIGRATION, [5, 5]])
+        expect(BackgroundMigrationWorker.jobs[2]['at']).to eq(3.minutes.from_now.to_f)
         expect(BackgroundMigrationWorker.jobs.size).to eq 3
       end
     end