diff --git a/.rubocop_todo/gitlab/bounded_contexts.yml b/.rubocop_todo/gitlab/bounded_contexts.yml index affb6b5d2e617860884c418224ece57c1d63408d..ca3017dff6711293b2c1b42001423dac38f0ef30 100644 --- a/.rubocop_todo/gitlab/bounded_contexts.yml +++ b/.rubocop_todo/gitlab/bounded_contexts.yml @@ -3431,7 +3431,6 @@ Gitlab/BoundedContexts: - 'ee/app/workers/concerns/elastic/migration_obsolete.rb' - 'ee/app/workers/concerns/elastic/migration_options.rb' - 'ee/app/workers/concerns/elastic/migration_state.rb' - - 'ee/app/workers/concerns/elastic/migration_update_mappings_helper.rb' - 'ee/app/workers/concerns/geo_backoff_delay.rb' - 'ee/app/workers/concerns/geo_queue.rb' - 'ee/app/workers/concerns/update_orchestration_policy_configuration.rb' diff --git a/.rubocop_todo/search/namespaced_class.yml b/.rubocop_todo/search/namespaced_class.yml index 3d4dee458e4cd31061ccb40315fcbd45cc03f6da..fe367321fefd20b0203b97b0c15289cb8f8c0658 100644 --- a/.rubocop_todo/search/namespaced_class.yml +++ b/.rubocop_todo/search/namespaced_class.yml @@ -47,7 +47,6 @@ Search/NamespacedClass: - 'ee/app/workers/concerns/elastic/migration_obsolete.rb' - 'ee/app/workers/concerns/elastic/migration_options.rb' - 'ee/app/workers/concerns/elastic/migration_state.rb' - - 'ee/app/workers/concerns/elastic/migration_update_mappings_helper.rb' - 'ee/app/workers/elastic/migration_worker.rb' - 'ee/app/workers/elastic/namespace_update_worker.rb' - 'ee/app/workers/elastic/project_transfer_worker.rb' diff --git a/doc/development/search/advanced_search_migration_styleguide.md b/doc/development/search/advanced_search_migration_styleguide.md index a32d6fe8b84c1ade48d34da71160afc121f742f7..778ae9f5b9cfe3cbb226acb4d72f8c74323b3eed 100644 --- a/doc/development/search/advanced_search_migration_styleguide.md +++ b/doc/development/search/advanced_search_migration_styleguide.md @@ -101,7 +101,7 @@ To apply setting changes, for example adding an analyzer, either: To apply mapping changes, either: - Use a [zero-downtime reindexing migration](#zero-downtime-reindex-migration). -- Use an [update mapping migration](#elasticmigrationupdatemappingshelper) to change the mapping for the existing index and optionally a follow-up [backfill migration](#searchelasticmigrationbackfillhelper) to ensure all documents in the index has this field populated. +- Use an [update mapping migration](#searchelasticmigrationupdatemappingshelper) to change the mapping for the existing index and optionally a follow-up [backfill migration](#searchelasticmigrationbackfillhelper) to ensure all documents in the index has this field populated. #### Zero-downtime reindex migration @@ -159,7 +159,7 @@ class MigrationName < Elastic::Migration end ``` -#### `Elastic::MigrationUpdateMappingsHelper` +#### `Search::Elastic::MigrationUpdateMappingsHelper` Updates a mapping in an index by calling `put_mapping` with the mapping specified. @@ -167,7 +167,7 @@ Requires the `new_mappings` method and `DOCUMENT_TYPE` constant. ```ruby class MigrationName < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper DOCUMENT_TYPE = Issue @@ -183,6 +183,14 @@ class MigrationName < Elastic::Migration end ``` +You can test this migration with the `'migration adds mapping'` shared examples. + +```ruby +describe 'migration', :elastic, :sidekiq_inline do + include_examples 'migration adds mapping' +end +``` + #### `Search::Elastic::MigrationRemoveFieldsHelper` Removes specified fields from an index. @@ -483,7 +491,7 @@ Use the following formula to calculate the runtime: Follow these best practices for best results: - Order all migrations for each document type so that any migrations that use - [`Elastic::MigrationUpdateMappingsHelper`](#elasticmigrationupdatemappingshelper) + [`Search::Elastic::MigrationUpdateMappingsHelper`](#searchelasticmigrationupdatemappingshelper) are executed before migrations that use the [`Search::Elastic::MigrationBackfillHelper`](#searchelasticmigrationbackfillhelper). This avoids reindexing the same documents multiple times if all of the migrations are unapplied diff --git a/ee/app/workers/concerns/elastic/migration_update_mappings_helper.rb b/ee/app/workers/concerns/elastic/migration_update_mappings_helper.rb deleted file mode 100644 index f8f669182dac54fa0e1ecdfc77ead067c4df72bb..0000000000000000000000000000000000000000 --- a/ee/app/workers/concerns/elastic/migration_update_mappings_helper.rb +++ /dev/null @@ -1,40 +0,0 @@ -# frozen_string_literal: true - -module Elastic - module MigrationUpdateMappingsHelper - def migrate - if completed? - log "Skipping updating #{index_name} mapping migration since it is already applied" - return - end - - log "Adding #{new_mappings.inspect} to #{index_name} mapping" - update_mapping!(index_name, { properties: new_mappings }) - end - - def completed? - helper.refresh_index(index_name: index_name) - - mappings = helper.get_mapping(index_name: index_name) - - # Check if mappings include all new_mappings - new_mappings.keys.map(&:to_s).to_set.subset?(mappings.keys.to_set) - end - - private - - def index_name - return self.class::DOCUMENT_TYPE.__elasticsearch__.index_name if self.class.const_defined?(:DOCUMENT_TYPE) - - raise NotImplementedError - end - - def new_mappings - raise NotImplementedError - end - - def update_mapping!(index_name, mappings) - helper.update_mapping(index_name: index_name, mappings: mappings) - end - end -end diff --git a/ee/app/workers/concerns/search/elastic/migration_update_mappings_helper.rb b/ee/app/workers/concerns/search/elastic/migration_update_mappings_helper.rb new file mode 100644 index 0000000000000000000000000000000000000000..0239a29434893ade00d49495f1e608b14266bdf3 --- /dev/null +++ b/ee/app/workers/concerns/search/elastic/migration_update_mappings_helper.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +module Search + module Elastic + module MigrationUpdateMappingsHelper + def migrate + if completed? + log "Skipping updating #{index_name} mapping migration since it is already applied" + return + end + + log "Adding #{new_mappings.inspect} to #{index_name} mapping" + update_mapping!(index_name, { properties: new_mappings }) + end + + def completed? + helper.refresh_index(index_name: index_name) + + mappings = helper.get_mapping(index_name: index_name) + + # Check if mappings include all new_mappings + new_mappings.keys.map(&:to_s).to_set.subset?(mappings.keys.to_set) + end + + private + + def index_name + return self.class::DOCUMENT_TYPE.__elasticsearch__.index_name if self.class.const_defined?(:DOCUMENT_TYPE) + + raise NotImplementedError + end + + def new_mappings + raise NotImplementedError + end + + def update_mapping!(index_name, mappings) + helper.update_mapping(index_name: index_name, mappings: mappings) + end + end + end +end diff --git a/ee/elastic/migrate/20231130152447_add_work_item_type_id_to_issues.rb b/ee/elastic/migrate/20231130152447_add_work_item_type_id_to_issues.rb index da29ea56f593539258f6154a50219db19b65a8b6..ba236f547a22880bd8ea6bdf8fad402293c12a15 100644 --- a/ee/elastic/migrate/20231130152447_add_work_item_type_id_to_issues.rb +++ b/ee/elastic/migrate/20231130152447_add_work_item_type_id_to_issues.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddWorkItemTypeIdToIssues < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper private diff --git a/ee/elastic/migrate/20240130154724_add_fields_to_projects_index.rb b/ee/elastic/migrate/20240130154724_add_fields_to_projects_index.rb index 55518954756c7e2b60bb6c41eee5da1264227470..35c968243a36107a858b59ebba47f24813cccf0d 100644 --- a/ee/elastic/migrate/20240130154724_add_fields_to_projects_index.rb +++ b/ee/elastic/migrate/20240130154724_add_fields_to_projects_index.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddFieldsToProjectsIndex < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper DOCUMENT_TYPE = Project diff --git a/ee/elastic/migrate/20240208160152_add_count_fields_to_projects.rb b/ee/elastic/migrate/20240208160152_add_count_fields_to_projects.rb index 7d247ac4b22b6d8a1b4126e07379a06bf01d7d30..e44e660a2988fcc1b7f24870049405abf8a14092 100644 --- a/ee/elastic/migrate/20240208160152_add_count_fields_to_projects.rb +++ b/ee/elastic/migrate/20240208160152_add_count_fields_to_projects.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddCountFieldsToProjects < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper DOCUMENT_TYPE = Project diff --git a/ee/elastic/migrate/20240410193847_add_embedding_to_issues.rb b/ee/elastic/migrate/20240410193847_add_embedding_to_issues.rb index 85222e5ac066f11068af8c45d2e717377886d329..b3af98b3715591353f3f492b759056f04226b70c 100644 --- a/ee/elastic/migrate/20240410193847_add_embedding_to_issues.rb +++ b/ee/elastic/migrate/20240410193847_add_embedding_to_issues.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddEmbeddingToIssues < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper skip_if -> { !Gitlab::Elastic::Helper.default.vectors_supported?(:elasticsearch) } diff --git a/ee/elastic/migrate/20240517092224_add_routing_to_issues.rb b/ee/elastic/migrate/20240517092224_add_routing_to_issues.rb index ff8fe2dfe97ca914563b89a952a84d24063a495c..15fe1b20cee62c1aa0ee62e985292c70fc4f6708 100644 --- a/ee/elastic/migrate/20240517092224_add_routing_to_issues.rb +++ b/ee/elastic/migrate/20240517092224_add_routing_to_issues.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddRoutingToIssues < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper DOCUMENT_TYPE = Issue diff --git a/ee/elastic/migrate/20240626145458_add_root_namespace_id_to_work_item.rb b/ee/elastic/migrate/20240626145458_add_root_namespace_id_to_work_item.rb index e17929a2b770e1f85dca5822c2636d51a1bd3fed..29781a7797ff2a5dc6b61f7bfd3b8ad7cf1a5e68 100644 --- a/ee/elastic/migrate/20240626145458_add_root_namespace_id_to_work_item.rb +++ b/ee/elastic/migrate/20240626145458_add_root_namespace_id_to_work_item.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddRootNamespaceIdToWorkItem < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper private diff --git a/ee/elastic/migrate/20240704125425_add_label_ids_to_merge_request.rb b/ee/elastic/migrate/20240704125425_add_label_ids_to_merge_request.rb index 9f8e3d8eddda1a1f55be744d0ba81a643eabcc45..03a862bc201587c3ebecec42fea7e44b9852f4a0 100644 --- a/ee/elastic/migrate/20240704125425_add_label_ids_to_merge_request.rb +++ b/ee/elastic/migrate/20240704125425_add_label_ids_to_merge_request.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddLabelIdsToMergeRequest < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper DOCUMENT_TYPE = MergeRequest diff --git a/ee/elastic/migrate/20240814223217_add_issues_access_level_in_work_item_index.rb b/ee/elastic/migrate/20240814223217_add_issues_access_level_in_work_item_index.rb index 7596a4a0c92ff2ff310a06f00269729c55f0df12..9814103793b512b36c2820b5fb6fa7198891dc52 100644 --- a/ee/elastic/migrate/20240814223217_add_issues_access_level_in_work_item_index.rb +++ b/ee/elastic/migrate/20240814223217_add_issues_access_level_in_work_item_index.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddIssuesAccessLevelInWorkItemIndex < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper private diff --git a/ee/elastic/migrate/20241003142503_add_embedding_to_work_items.rb b/ee/elastic/migrate/20241003142503_add_embedding_to_work_items.rb index dffedeb5f937d0fea2307e74c79d6be53ca9fb12..d70465229181d0f90007ddddd8c66033ff7b27e6 100644 --- a/ee/elastic/migrate/20241003142503_add_embedding_to_work_items.rb +++ b/ee/elastic/migrate/20241003142503_add_embedding_to_work_items.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddEmbeddingToWorkItems < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper skip_if -> { !elasticsearch_8_plus? } diff --git a/ee/elastic/migrate/20241003151804_add_notes_to_work_items.rb b/ee/elastic/migrate/20241003151804_add_notes_to_work_items.rb index 42dbae37d0087efde99fac92682ecb1facb416bd..a84584c0c37ca52521b019c94daba55e9eb1381c 100644 --- a/ee/elastic/migrate/20241003151804_add_notes_to_work_items.rb +++ b/ee/elastic/migrate/20241003151804_add_notes_to_work_items.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddNotesToWorkItems < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper private diff --git a/ee/elastic/migrate/20241105111645_add_work_item_type_correct_id.rb b/ee/elastic/migrate/20241105111645_add_work_item_type_correct_id.rb index de0449ce0bfc15edc5b8fc3f7bd49ca94ea9e090..5474e9c6a7c16b5f7f72d8da7595969381ca50bc 100644 --- a/ee/elastic/migrate/20241105111645_add_work_item_type_correct_id.rb +++ b/ee/elastic/migrate/20241105111645_add_work_item_type_correct_id.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddWorkItemTypeCorrectId < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper DOCUMENT_TYPE = WorkItem diff --git a/ee/elastic/migrate/20241107131942_add_traversal_ids_to_merge_requests.rb b/ee/elastic/migrate/20241107131942_add_traversal_ids_to_merge_requests.rb index 8b1e39e1d20ea497f52e14eeba2ff9366ee29d31..1da209bf4268edc0e73ccbf8dded0ef00dc08e9c 100644 --- a/ee/elastic/migrate/20241107131942_add_traversal_ids_to_merge_requests.rb +++ b/ee/elastic/migrate/20241107131942_add_traversal_ids_to_merge_requests.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddTraversalIdsToMergeRequests < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper DOCUMENT_TYPE = MergeRequest diff --git a/ee/elastic/migrate/20241204115350_add_assignees_to_merge_requests.rb b/ee/elastic/migrate/20241204115350_add_assignees_to_merge_requests.rb index bc51bab7e6a52ffdb75308c44fb48b4378b09547..97743d471aad0f1f158d31a9f0d404f1471149d3 100644 --- a/ee/elastic/migrate/20241204115350_add_assignees_to_merge_requests.rb +++ b/ee/elastic/migrate/20241204115350_add_assignees_to_merge_requests.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AddAssigneesToMergeRequests < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper DOCUMENT_TYPE = MergeRequest diff --git a/ee/spec/services/elastic/data_migration_service_spec.rb b/ee/spec/services/elastic/data_migration_service_spec.rb index 867d0324140cd4c27bf8ad7d679e589aad34f621..cb263d6fe914fd151a5bc1bc08ecd9bf2fbee756 100644 --- a/ee/spec/services/elastic/data_migration_service_spec.rb +++ b/ee/spec/services/elastic/data_migration_service_spec.rb @@ -67,7 +67,7 @@ describe 'migrations order optimization' do it 'ensure all update migrations run before backfill migrations', :aggregate_failures do error_message = <<~DOC - Migrations should be ordered so all migrations that use ::Elastic::MigrationUpdateMappingsHelper + Migrations should be ordered so all migrations that use ::Search::Elastic::MigrationUpdateMappingsHelper run before any migrations that use ::Search::Elastic::MigrationBackfillHelper. If this spec fails, rename the `YYYYMMDDHHMMSS` part of the migration filename with a datetime before the last backfill migration for the index_name. @@ -78,7 +78,7 @@ filtered_migrations = non_obsolete_migrations.filter do |m| klass = m.class - klass.include?(::Elastic::MigrationUpdateMappingsHelper) || + klass.include?(::Search::Elastic::MigrationUpdateMappingsHelper) || klass.include?(::Search::Elastic::MigrationBackfillHelper) end @@ -90,7 +90,7 @@ end.map(&:version) mapping_versions = non_obsolete_migrations.filter do |m| - m.class.include?(::Elastic::MigrationUpdateMappingsHelper) && m.send(:index_name) == index_name + m.class.include?(::Search::Elastic::MigrationUpdateMappingsHelper) && m.send(:index_name) == index_name end.map(&:version) backfill_ranges = backfill_versions.each_cons(2).map { |a, b| a..b } diff --git a/spec/fixtures/migrations/elasticsearch/elasticsearch_migration.txt b/spec/fixtures/migrations/elasticsearch/elasticsearch_migration.txt index 122d3ef6d2b36a72c5b7110b256e652ec88deb03..2418ee88ffd08af9ec505add4ccff33a7457ed93 100644 --- a/spec/fixtures/migrations/elasticsearch/elasticsearch_migration.txt +++ b/spec/fixtures/migrations/elasticsearch/elasticsearch_migration.txt @@ -1,7 +1,7 @@ # frozen_string_literal: true class ElasticsearchMigration < Elastic::Migration - include Elastic::MigrationUpdateMappingsHelper + include ::Search::Elastic::MigrationUpdateMappingsHelper private