From acc6b3a1ff73595d68b76fa4966d87c573957a0e Mon Sep 17 00:00:00 2001 From: Dmitry Gruzd <dgruzd@gitlab.com> Date: Thu, 11 Feb 2021 14:34:42 +0300 Subject: [PATCH] Simplify the execute_migration method --- ee/app/models/elastic/migration_record.rb | 4 ++-- ee/app/workers/elastic/migration_worker.rb | 22 ++++++++++--------- .../models/elastic/migration_record_spec.rb | 14 ++++++------ .../elastic/data_migration_service_spec.rb | 4 ++-- ee/spec/tasks/gitlab/elastic_rake_spec.rb | 4 ++-- .../workers/elastic/migration_worker_spec.rb | 6 ++--- 6 files changed, 28 insertions(+), 26 deletions(-) diff --git a/ee/app/models/elastic/migration_record.rb b/ee/app/models/elastic/migration_record.rb index 5f56b8a0322..3c2485141fe 100644 --- a/ee/app/models/elastic/migration_record.rb +++ b/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 } } }) diff --git a/ee/app/workers/elastic/migration_worker.rb b/ee/app/workers/elastic/migration_worker.rb index b4fe516d52f..1bc833e696c 100644 --- a/ee/app/workers/elastic/migration_worker.rb +++ b/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 diff --git a/ee/spec/models/elastic/migration_record_spec.rb b/ee/spec/models/elastic/migration_record_spec.rb index f6e4588d688..89fec5d76d1 100644 --- a/ee/spec/models/elastic/migration_record_spec.rb +++ b/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 diff --git a/ee/spec/services/elastic/data_migration_service_spec.rb b/ee/spec/services/elastic/data_migration_service_spec.rb index c2d0cbdb36c..d038ef851c3 100644 --- a/ee/spec/services/elastic/data_migration_service_spec.rb +++ b/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 diff --git a/ee/spec/tasks/gitlab/elastic_rake_spec.rb b/ee/spec/tasks/gitlab/elastic_rake_spec.rb index 717cf50e316..a3d3ccb24c0 100644 --- a/ee/spec/tasks/gitlab/elastic_rake_spec.rb +++ b/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 diff --git a/ee/spec/workers/elastic/migration_worker_spec.rb b/ee/spec/workers/elastic/migration_worker_spec.rb index ec6d2499741..916717f466c 100644 --- a/ee/spec/workers/elastic/migration_worker_spec.rb +++ b/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 -- 2.30.9