Remove indexing for mysql. with_legacy_artifacts targets all archive rows. Enhance tests.

db/post_migrate/20180430161409_migrate_legacy_artifacts_to_job_artifacts.rb

@@ -10,19 +10,13 @@ class MigrateLegacyArtifactsToJobArtifacts < ActiveRecord::Migration
   def up
     ##
-    # We add a temporary index to `artifacts_file`. Without having this,
-    # queries generated by `each_batch` by statement timeout.
-    #
-    # This is only neccessary when we collect target rows, because in backgroun migrartions
-    # it's filitered by `BETWEEN` clause (e.g. 'id BETWEEN 0 AND 2000'), so it's fast enough without this temporary index.
-    #
-    # Mysql doesn't support partial indexies. Also, it requires the index to specify `length` only if the type is `TEXT`, `VARCHAR`, etc.
-    # Therefore we need to treat this id generation differently by database type.
+    # We add a temporary index to `artifacts_file`. If we don't have the index,
+    # the first query (`SELECT .. WHERE .. ORDER BY id ASC LIMIT 1``) of `each_batch` will likely fail by statement timeout.
+    # The following querires which will be executed in backgroun migrartions are fine without the index,
+    # because it's scanned by using `BETWEEN` clause (e.g. 'id BETWEEN 0 AND 2000') at the first place.
     unless index_exists_by_name?(:ci_builds, TMP_INDEX)
       if Gitlab::Database.postgresql?
         add_concurrent_index :ci_builds, :artifacts_file, where: "artifacts_file <> ''", name: TMP_INDEX
-      elsif Gitlab::Database.mysql?
-        add_concurrent_index :ci_builds, :artifacts_file, length: 20, name: TMP_INDEX
       end
     end

lib/gitlab/background_migration/migrate_legacy_artifacts.rb

@@ -11,7 +11,7 @@ module Gitlab
         self.table_name = 'ci_builds'
         self.inheritance_column = :_type_disabled
-        scope :with_legacy_artifacts, -> { where("artifacts_file <> '' AND artifacts_metadata <> ''") }
+        scope :with_legacy_artifacts, -> { where("artifacts_file <> ''") }
         scope :without_new_artifacts, -> do
           where('NOT EXISTS (SELECT 1 FROM ci_job_artifacts WHERE (ci_builds.id = ci_job_artifacts.job_id) AND ci_job_artifacts.file_type = 1)')

spec/lib/gitlab/background_migration/migrate_legacy_artifacts_spec.rb

@@ -81,15 +81,27 @@ describe Gitlab::BackgroundMigration::MigrateLegacyArtifacts, :migration, schema
         end
         context 'when new artifacts has already existed' do
-          before do
-            job_artifacts.create!(id: 1, project_id: project_id, job_id: job_id, file_type: 1, size: 123, file: 'archive.zip')
-            job_artifacts.create!(id: 2, project_id: project_id, job_id: job_id, file_type: 2, size: 123, file: 'metadata.tar.gz')
+          context 'when only archive existed' do
+            before do
+              job_artifacts.create!(id: 1, project_id: project_id, job_id: job_id, file_type: 1, size: 123, file: 'archive.zip')
+            end
+
+            it 'migrates metadata too' do
+              described_class.new.perform(*range)
+
+              expect(job_artifacts.where(job_id: job_id, file_type: 2).pluck('file')).to eq(['metadata.gz'])
+            end
           end
-          it 'does not migrate' do
-            described_class.new.perform(*range)
-            expect(job_artifacts.pluck('id')).to eq([1, 2])
+          context 'when both archive and metadata existed' do
+            before do
+              job_artifacts.create!(id: 1, project_id: project_id, job_id: job_id, file_type: 1, size: 123, file: 'archive.zip')
+              job_artifacts.create!(id: 2, project_id: project_id, job_id: job_id, file_type: 2, size: 123, file: 'metadata.gz')
+            end
+            it 'does not migrate' do
+              expect { described_class.new.perform(*range) }.not_to change { job_artifacts.count }
+            end
           end
         end
       end

spec/migrations/migrate_legacy_artifacts_to_job_artifacts_spec.rb

@@ -19,11 +19,12 @@ describe MigrateLegacyArtifactsToJobArtifacts, :migration, :sidekiq 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')
+      jobs.create!(id: 1, commit_id: 1, project_id: 1, status: :success, artifacts_file: 'archive.zip')
+      jobs.create!(id: 2, commit_id: 1, project_id: 1, status: :failed, artifacts_metadata: 'metadata.gz')
+      jobs.create!(id: 3, commit_id: 1, project_id: 1, status: :failed, artifacts_file: 'archive.zip', artifacts_metadata: 'metadata.gz')
+      jobs.create!(id: 4, commit_id: 1, project_id: 1, status: :running)
+      jobs.create!(id: 5, commit_id: 1, project_id: 1, status: :success, artifacts_file: 'archive.zip', artifacts_metadata: 'metadata.gz')
+      jobs.create!(id: 6, commit_id: 1, project_id: 1, status: :failed, artifacts_file: 'archive.zip', artifacts_metadata: 'metadata.gz')
     end
     it 'schedules a background migration' do
@@ -31,7 +32,7 @@ describe MigrateLegacyArtifactsToJobArtifacts, :migration, :sidekiq do
         Timecop.freeze do
           migrate!
-          expect(migration_name).to be_scheduled_delayed_migration(5.minutes, 1, 5)
+          expect(migration_name).to be_scheduled_delayed_migration(5.minutes, 1, 6)
           expect(BackgroundMigrationWorker.jobs.size).to eq 1
         end
       end
@@ -39,16 +40,16 @@ 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')
+        job_artifacts.create!(job_id: 1, id: 1, project_id: 1, file_type: 1, size: 123, file: 'archive.zip')
+        job_artifacts.create!(job_id: 6, id: 2, project_id: 1, file_type: 1, size: 123, file: 'archive.zip')
       end
-      it 'does not schedule background migrations' do
+      it 'schedules a background migration to specific targets' do
         Sidekiq::Testing.fake! do
           Timecop.freeze do
             migrate!
-            expect(migration_name).to be_scheduled_delayed_migration(5.minutes, 2, 4)
+            expect(migration_name).to be_scheduled_delayed_migration(5.minutes, 3, 5)
             expect(BackgroundMigrationWorker.jobs.size).to eq 1
           end
         end
@@ -59,6 +60,7 @@ describe MigrateLegacyArtifactsToJobArtifacts, :migration, :sidekiq do
   context 'when legacy artifacts do not exist' do
     before do
       jobs.create!(id: 1, commit_id: 1, project_id: 1, status: :success)
+      jobs.create!(id: 2, commit_id: 1, project_id: 1, status: :failed, artifacts_metadata: 'metadata.gz')
     end
     it 'does not schedule background migrations' do