Merge branch '290763-simplify-execute-migration' into 'master'

Simplify the execute_migration method

See merge request gitlab-org/gitlab!53824

ee/app/models/elastic/migration_record.rb

@@ -27,7 +27,7 @@ module Elastic
       client.index index: index_name, type: '_doc', id: version, body: { completed: completed, state: load_state.merge(state) }
     end
 
-    def persisted?
+    def started?
       load_from_index.present?
     end
 
@@ -49,7 +49,7 @@ module Elastic
       name.underscore
     end
 
-    def self.persisted_versions(completed:)
+    def self.load_versions(completed:)
       helper = Gitlab::Elastic::Helper.default
       helper.client
             .search(index: helper.migrations_index_name, body: { query: { term: { completed: completed } } })

ee/app/workers/elastic/migration_worker.rb

@@ -51,23 +51,25 @@ module Elastic
     private
 
     def execute_migration(migration)
-      if migration.persisted? && !migration.batched?
+      if migration.started? && !migration.batched?
         logger.info "MigrationWorker: migration[#{migration.name}] did not execute migrate method since it was already executed. Waiting for migration to complete"
-      else
-        pause_indexing!(migration)
 
-        logger.info "MigrationWorker: migration[#{migration.name}] executing migrate method"
-        migration.migrate
+        return
+      end
 
-        if migration.batched? && !migration.completed?
-          logger.info "MigrationWorker: migration[#{migration.name}] kicking off next migration batch"
-          Elastic::MigrationWorker.perform_in(migration.throttle_delay)
-        end
+      pause_indexing!(migration)
+
+      logger.info "MigrationWorker: migration[#{migration.name}] executing migrate method"
+      migration.migrate
+
+      if migration.batched? && !migration.completed?
+        logger.info "MigrationWorker: migration[#{migration.name}] kicking off next migration batch"
+        Elastic::MigrationWorker.perform_in(migration.throttle_delay)
       end
     end
 
     def current_migration
-      completed_migrations = Elastic::MigrationRecord.persisted_versions(completed: true)
+      completed_migrations = Elastic::MigrationRecord.load_versions(completed: true)
 
       Elastic::DataMigrationService.migrations.find { |migration| !completed_migrations.include?(migration.version) }
     end

ee/spec/models/elastic/migration_record_spec.rb

@@ -41,13 +41,13 @@ RSpec.describe Elastic::MigrationRecord, :elastic do
     end
   end
 
-  describe '#persisted?' do
+  describe '#started?' do
     it 'changes on object save' do
-      expect { record.save!(completed: true) }.to change { record.persisted? }.from(false).to(true)
+      expect { record.save!(completed: true) }.to change { record.started? }.from(false).to(true)
     end
   end
 
-  describe '.persisted_versions' do
+  describe '.load_versions' do
     let(:completed_versions) { 1.upto(5).map { |i| described_class.new(version: i, name: i, filename: nil) } }
     let(:in_progress_migration) { described_class.new(version: 10, name: 10, filename: nil) }
 
@@ -61,15 +61,15 @@ RSpec.describe Elastic::MigrationRecord, :elastic do
     end
 
     it 'loads all records' do
-      expect(described_class.persisted_versions(completed: true)).to match_array(completed_versions.map(&:version))
-      expect(described_class.persisted_versions(completed: false)).to contain_exactly(in_progress_migration.version)
+      expect(described_class.load_versions(completed: true)).to match_array(completed_versions.map(&:version))
+      expect(described_class.load_versions(completed: false)).to contain_exactly(in_progress_migration.version)
     end
 
     it 'returns empty array if no index present' do
       es_helper.delete_migrations_index
 
-      expect(described_class.persisted_versions(completed: true)).to eq([])
-      expect(described_class.persisted_versions(completed: false)).to eq([])
+      expect(described_class.load_versions(completed: true)).to eq([])
+      expect(described_class.load_versions(completed: false)).to eq([])
     end
   end
 end

ee/spec/services/elastic/data_migration_service_spec.rb

@@ -75,12 +75,12 @@ RSpec.describe Elastic::DataMigrationService, :elastic do
     end
 
     it 'creates all migration versions' do
-      expect(Elastic::MigrationRecord.persisted_versions(completed: true).count).to eq(0)
+      expect(Elastic::MigrationRecord.load_versions(completed: true).count).to eq(0)
 
       subject.mark_all_as_completed!
       refresh_index!
 
-      expect(Elastic::MigrationRecord.persisted_versions(completed: true).count).to eq(subject.migrations.count)
+      expect(Elastic::MigrationRecord.load_versions(completed: true).count).to eq(subject.migrations.count)
     end
 
     it 'drops all cache keys' do

ee/spec/tasks/gitlab/elastic_rake_spec.rb

@@ -69,13 +69,13 @@ RSpec.describe 'gitlab:elastic namespace rake tasks', :elastic do
 
     it 'marks all migrations as completed' do
       expect(Elastic::DataMigrationService).to receive(:mark_all_as_completed!).and_call_original
-      expect(Elastic::MigrationRecord.persisted_versions(completed: true)).to eq([])
+      expect(Elastic::MigrationRecord.load_versions(completed: true)).to eq([])
 
       subject
       refresh_index!
 
       migrations = Elastic::DataMigrationService.migrations.map(&:version)
-      expect(Elastic::MigrationRecord.persisted_versions(completed: true)).to eq(migrations)
+      expect(Elastic::MigrationRecord.load_versions(completed: true)).to eq(migrations)
     end
   end
 

ee/spec/workers/elastic/migration_worker_spec.rb

@@ -77,7 +77,7 @@ RSpec.describe Elastic::MigrationWorker, :elastic do
 
       context 'migration process' do
         before do
-          allow(migration).to receive(:persisted?).and_return(persisted)
+          allow(migration).to receive(:started?).and_return(started)
           allow(migration).to receive(:completed?).and_return(completed)
           allow(migration).to receive(:batched?).and_return(batched)
         end
@@ -85,7 +85,7 @@ RSpec.describe Elastic::MigrationWorker, :elastic do
         using RSpec::Parameterized::TableSyntax
 
         # completed is evaluated after migrate method is executed
-        where(:persisted, :completed, :execute_migration, :batched) do
+        where(:started, :completed, :execute_migration, :batched) do
           false | false | true  | false
           false | true  | true  | false
           false | false | true  | true
@@ -130,7 +130,7 @@ RSpec.describe Elastic::MigrationWorker, :elastic do
 
           let(:batched) { true }
 
-          where(:persisted, :completed, :expected) do
+          where(:started, :completed, :expected) do
             false | false | false
             true  | false | false
             true  | true  | true