Optimize queries. Add some tests for filtering logic.

db/post_migrate/20180427161409_migrate_legacy_artifacts_to_job_artifacts.rb

@@ -12,19 +12,23 @@ class MigrateLegacyArtifactsToJobArtifacts < ActiveRecord::Migration
     self.table_name = 'ci_builds'
     self.inheritance_column = :_type_disabled # disable STI
-    ##
-    # Jobs which have a value on `artifacts_file` column are targetted.
-    # In addition, jobs which have already had job_artifacts are untargetted.
-    # This usually doesn't happen, however, if it's the case, background migrations will be aborted
-    scope :legacy_artifacts, -> do
-      where('artifacts_file IS NOT NULL AND artifacts_file <> ?', '')
-    end
+    scope :legacy_artifacts, -> { where("artifacts_file <> ''") }
     scope :without_new_artifacts, -> do
-      where('NOT EXISTS (SELECT 1 FROM ci_job_artifacts WHERE ci_job_artifacts.id = ci_builds.id AND (file_type = 1 OR file_type = 2))')
+      where('NOT EXISTS (?)', MigrateLegacyArtifactsToJobArtifacts::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').archive)
     end
   end
+  class JobArtifact < ActiveRecord::Base
+    self.table_name = 'ci_job_artifacts'
+
+    enum file_type: {
+      archive: 1,
+      metadata: 2,
+      trace: 3
+    }
+  end
+
   def up
     disable_statement_timeout

lib/gitlab/background_migration/migrate_legacy_artifacts.rb

@@ -9,12 +9,10 @@ module Gitlab
         self.table_name = 'ci_builds'
         self.inheritance_column = :_type_disabled
-        scope :legacy_artifacts, -> do
-          where('artifacts_file IS NOT NULL AND artifacts_file <> ?', '')
-        end
+        scope :legacy_artifacts, -> { where("artifacts_file <> ''") }
         scope :without_new_artifacts, -> do
-          where('NOT EXISTS (SELECT 1 FROM ci_job_artifacts WHERE ci_job_artifacts.id = ci_builds.id AND (file_type = 1 OR file_type = 2))')
+          where('NOT EXISTS (?)', MigrateLegacyArtifacts::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').archive)
         end
       end

spec/migrations/migrate_legacy_artifacts_to_job_artifacts_spec.rb

@@ -17,9 +17,13 @@ describe MigrateLegacyArtifactsToJobArtifacts, :migration, :sidekiq do
     pipelines.create!(id: 1, project_id: 1, ref: 'master', sha: 'adf43c3a')
   end
-  context 'when a legacy artifacts exists' do
+  context 'when legacy artifacts exist' do
     before do
       jobs.create!(id: 1, commit_id: 1, project_id: 1, status: :success, artifacts_file: 'archive.zip', artifacts_metadata: 'metadata.gz')
+      jobs.create!(id: 2, commit_id: 1, project_id: 1, status: :failed, artifacts_file: 'archive.zip', artifacts_metadata: 'metadata.gz')
+      jobs.create!(id: 3, commit_id: 1, project_id: 1, status: :running)
+      jobs.create!(id: 4, commit_id: 1, project_id: 1, status: :success, artifacts_file: 'archive.zip', artifacts_metadata: 'metadata.gz')
+      jobs.create!(id: 5, commit_id: 1, project_id: 1, status: :failed, artifacts_file: 'archive.zip', artifacts_metadata: 'metadata.gz')
     end
     it 'schedules a background migration' do
@@ -27,7 +31,7 @@ describe MigrateLegacyArtifactsToJobArtifacts, :migration, :sidekiq do
         Timecop.freeze do
           migrate!
-          expect(migration_name).to be_scheduled_delayed_migration(5.minutes, 1, 1)
+          expect(migration_name).to be_scheduled_delayed_migration(5.minutes, 1, 5)
           expect(BackgroundMigrationWorker.jobs.size).to eq 1
         end
       end
@@ -36,13 +40,17 @@ describe MigrateLegacyArtifactsToJobArtifacts, :migration, :sidekiq do
     context 'when new artifacts has already existed' do
       before do
         job_artifacts.create!(id: 1, project_id: 1, job_id: 1, file_type: 1, size: 123, file: 'archive.zip')
+        job_artifacts.create!(id: 2, project_id: 1, job_id: 5, file_type: 1, size: 123, file: 'archive.zip')
       end
       it 'does not schedule background migrations' do
         Sidekiq::Testing.fake! do
-          migrate!
-          expect(BackgroundMigrationWorker.jobs.size).to eq 0
+          Timecop.freeze do
+            migrate!
+            expect(migration_name).to be_scheduled_delayed_migration(5.minutes, 2, 4)
+            expect(BackgroundMigrationWorker.jobs.size).to eq 1
+          end
         end
       end
     end
@@ -55,9 +63,11 @@ describe MigrateLegacyArtifactsToJobArtifacts, :migration, :sidekiq do
     it 'does not schedule background migrations' do
       Sidekiq::Testing.fake! do
-        migrate!
-        expect(BackgroundMigrationWorker.jobs.size).to eq 0
+        Timecop.freeze do
+          migrate!
+          expect(BackgroundMigrationWorker.jobs.size).to eq 0
+        end
       end
     end
   end