From bedfda4f93fb9c42b671ff6670315086ac551228 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Thu, 25 Jan 2024 17:11:48 +0100 Subject: [PATCH 1/4] Introduce MAPPING_AUTO_UPDATE merge reason This is in preparation of https://github.com/elastic/elasticsearch/pull/96235. At the moment, there's no difference between MAPPING_AUTO_UPDATE and MAPPING_AUTO_UPDATE_PREFLIGHT. After the other PR is merged, when the merge reason is auto-update and if ignore_dynamic_beyond_limit is set, the merge process will only add dynamically mapped fields until the field limit is reached and ignores additional ones. --- .../PutMappingClusterStateUpdateRequest.java | 9 ++++++ .../put/TransportAutoPutMappingAction.java | 2 +- .../put/TransportPutMappingAction.java | 8 ++++-- .../action/bulk/TransportShardBulkAction.java | 2 +- .../metadata/MetadataMappingService.java | 2 +- .../index/mapper/MapperService.java | 28 +++++++++++++++---- .../index/mapper/MapperServiceTests.java | 2 +- 7 files changed, 41 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingClusterStateUpdateRequest.java b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingClusterStateUpdateRequest.java index 635412c8be2e8..c8f80eae99a3d 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingClusterStateUpdateRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingClusterStateUpdateRequest.java @@ -19,6 +19,7 @@ public class PutMappingClusterStateUpdateRequest extends IndicesClusterStateUpdateRequest { private final CompressedXContent source; + private boolean autoUpdate; public PutMappingClusterStateUpdateRequest(String source) throws IOException { this.source = CompressedXContent.fromJSON(source); @@ -28,4 +29,12 @@ public CompressedXContent source() { return source; } + public PutMappingClusterStateUpdateRequest autoUpdate(boolean autoUpdate) { + this.autoUpdate = autoUpdate; + return this; + } + + public boolean autoUpdate() { + return autoUpdate; + } } diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportAutoPutMappingAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportAutoPutMappingAction.java index a94b1fce0439f..9c3b08ef49add 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportAutoPutMappingAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportAutoPutMappingAction.java @@ -93,7 +93,7 @@ protected void masterOperation( return; } - performMappingUpdate(concreteIndices, request, listener, metadataMappingService); + performMappingUpdate(concreteIndices, request, listener, metadataMappingService, true); } } diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportPutMappingAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportPutMappingAction.java index 0b21cd8de86fb..e93bc1501744d 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportPutMappingAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportPutMappingAction.java @@ -112,7 +112,7 @@ protected void masterOperation( return; } - performMappingUpdate(concreteIndices, request, listener, metadataMappingService); + performMappingUpdate(concreteIndices, request, listener, metadataMappingService, false); } catch (IndexNotFoundException ex) { logger.debug(() -> "failed to put mappings on indices [" + Arrays.asList(request.indices() + "]"), ex); throw ex; @@ -147,7 +147,8 @@ static void performMappingUpdate( Index[] concreteIndices, PutMappingRequest request, ActionListener listener, - MetadataMappingService metadataMappingService + MetadataMappingService metadataMappingService, + boolean autoUpdate ) { final ActionListener wrappedListener = listener.delegateResponse((l, e) -> { logger.debug(() -> "failed to put mappings on indices [" + Arrays.asList(concreteIndices) + "]", e); @@ -157,7 +158,8 @@ static void performMappingUpdate( try { updateRequest = new PutMappingClusterStateUpdateRequest(request.source()).indices(concreteIndices) .ackTimeout(request.timeout()) - .masterNodeTimeout(request.masterNodeTimeout()); + .masterNodeTimeout(request.masterNodeTimeout()) + .autoUpdate(autoUpdate); } catch (IOException e) { wrappedListener.onFailure(e); return; diff --git a/server/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java b/server/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java index ee9126622efc9..2ce276a7c0524 100644 --- a/server/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java +++ b/server/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java @@ -377,7 +377,7 @@ static boolean executeBulkItemRequest( .merge( MapperService.SINGLE_MAPPING_NAME, new CompressedXContent(result.getRequiredMappingUpdate()), - MapperService.MergeReason.MAPPING_UPDATE_PREFLIGHT + MapperService.MergeReason.MAPPING_AUTO_UPDATE_PREFLIGHT ) ).map(DocumentMapper::mappingSource); Optional previousSource = Optional.ofNullable(primary.mapperService().documentMapper()) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java index 7a2d20d042f84..71b0ec9467158 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java @@ -167,7 +167,7 @@ private static ClusterState applyRequest( DocumentMapper mergedMapper = mapperService.merge( MapperService.SINGLE_MAPPING_NAME, mappingUpdateSource, - MergeReason.MAPPING_UPDATE + request.autoUpdate() ? MergeReason.MAPPING_AUTO_UPDATE : MergeReason.MAPPING_UPDATE ); CompressedXContent updatedSource = mergedMapper.mappingSource(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 61dd444be34b1..20bd7cf4a87e6 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -56,9 +56,23 @@ public class MapperService extends AbstractIndexComponent implements Closeable { */ public enum MergeReason { /** - * Pre-flight check before sending a mapping update to the master + * Pre-flight check before sending a dynamic mapping update to the master */ - MAPPING_UPDATE_PREFLIGHT, + MAPPING_AUTO_UPDATE_PREFLIGHT { + @Override + public boolean isAutoUpdate() { + return true; + } + }, + /** + * Dynamic mapping updates + */ + MAPPING_AUTO_UPDATE { + @Override + public boolean isAutoUpdate() { + return true; + } + }, /** * Create or update a mapping. */ @@ -72,7 +86,11 @@ public enum MergeReason { * if a shard was moved to a different node or for administrative * purposes. */ - MAPPING_RECOVERY + MAPPING_RECOVERY; + + public boolean isAutoUpdate() { + return false; + } } public static final String SINGLE_MAPPING_NAME = "_doc"; @@ -364,7 +382,7 @@ boolean assertNoUpdateRequired(final IndexMetadata newIndexMetadata) { } public void merge(IndexMetadata indexMetadata, MergeReason reason) { - assert reason != MergeReason.MAPPING_UPDATE_PREFLIGHT; + assert reason != MergeReason.MAPPING_AUTO_UPDATE_PREFLIGHT; MappingMetadata mappingMetadata = indexMetadata.mapping(); if (mappingMetadata != null) { merge(mappingMetadata.type(), mappingMetadata.source(), reason); @@ -521,7 +539,7 @@ private synchronized DocumentMapper doMerge(String type, MergeReason reason, Map // TODO: In many cases the source here is equal to mappingSource so we need not serialize again. // We should identify these cases reliably and save expensive serialization here DocumentMapper newMapper = newDocumentMapper(mapping, reason, mapping.toCompressedXContent()); - if (reason == MergeReason.MAPPING_UPDATE_PREFLIGHT) { + if (reason == MergeReason.MAPPING_AUTO_UPDATE_PREFLIGHT) { return newMapper; } this.mapper = newMapper; diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index aa3b083f4496f..ff2cabde8e0f9 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -37,7 +37,7 @@ public class MapperServiceTests extends MapperServiceTestCase { public void testPreflightUpdateDoesNotChangeMapping() throws Throwable { final MapperService mapperService = createMapperService(mapping(b -> {})); - merge(mapperService, MergeReason.MAPPING_UPDATE_PREFLIGHT, mapping(b -> createMappingSpecifyingNumberOfFields(b, 1))); + merge(mapperService, MergeReason.MAPPING_AUTO_UPDATE_PREFLIGHT, mapping(b -> createMappingSpecifyingNumberOfFields(b, 1))); assertThat("field was not created by preflight check", mapperService.fieldType("field0"), nullValue()); merge(mapperService, MergeReason.MAPPING_UPDATE, mapping(b -> createMappingSpecifyingNumberOfFields(b, 1))); assertThat("field was not created by mapping update", mapperService.fieldType("field0"), notNullValue()); From 5e5e039d157945a8153f4489b1f626eb5e22b766 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Fri, 26 Jan 2024 09:15:28 +0100 Subject: [PATCH 2/4] Randomly use MAPPING_AUTO_UPDATE in tests --- .../index/mapper/DocumentMapperTests.java | 31 +++++++++++++++---- .../index/mapper/MapperServiceTests.java | 6 +++- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java index e935ae2431131..aab481b545879 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java @@ -62,7 +62,7 @@ public void testAddFields() throws Exception { b.endObject(); })); - MergeReason reason = randomFrom(MergeReason.MAPPING_UPDATE, MergeReason.INDEX_TEMPLATE); + MergeReason reason = randomFrom(MergeReason.MAPPING_UPDATE, MergeReason.INDEX_TEMPLATE, MergeReason.MAPPING_AUTO_UPDATE); Mapping merged = MapperService.mergeMappings(stage1, stage2.mapping(), reason, Long.MAX_VALUE); // stage1 mapping should not have been modified assertThat(stage1.mappers().getMapper("age"), nullValue()); @@ -81,14 +81,19 @@ public void testMergeObjectDynamic() throws Exception { DocumentMapper withDynamicMapper = createDocumentMapper(topMapping(b -> b.field("dynamic", "false"))); assertThat(withDynamicMapper.mapping().getRoot().dynamic(), equalTo(ObjectMapper.Dynamic.FALSE)); - Mapping merged = MapperService.mergeMappings(mapper, withDynamicMapper.mapping(), MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); + Mapping merged = MapperService.mergeMappings( + mapper, + withDynamicMapper.mapping(), + randomFrom(MergeReason.MAPPING_UPDATE, MergeReason.MAPPING_AUTO_UPDATE), + Long.MAX_VALUE + ); assertThat(merged.getRoot().dynamic(), equalTo(ObjectMapper.Dynamic.FALSE)); } public void testMergeObjectAndNested() throws Exception { DocumentMapper objectMapper = createDocumentMapper(mapping(b -> b.startObject("obj").field("type", "object").endObject())); DocumentMapper nestedMapper = createDocumentMapper(mapping(b -> b.startObject("obj").field("type", "nested").endObject())); - MergeReason reason = randomFrom(MergeReason.MAPPING_UPDATE, MergeReason.INDEX_TEMPLATE); + MergeReason reason = randomFrom(MergeReason.MAPPING_UPDATE, MergeReason.INDEX_TEMPLATE, MergeReason.MAPPING_AUTO_UPDATE); { IllegalArgumentException e = expectThrows( @@ -178,7 +183,11 @@ public void testConcurrentMergeTest() throws Throwable { Mapping update = doc.dynamicMappingsUpdate(); assert update != null; lastIntroducedFieldName.set(fieldName); - mapperService.merge("_doc", new CompressedXContent(update.toString()), MergeReason.MAPPING_UPDATE); + mapperService.merge( + "_doc", + new CompressedXContent(update.toString()), + randomFrom(MergeReason.MAPPING_UPDATE, MergeReason.MAPPING_AUTO_UPDATE) + ); } } catch (Exception e) { error.set(e); @@ -235,11 +244,21 @@ public void testMergeMeta() throws IOException { DocumentMapper updatedMapper = createDocumentMapper(fieldMapping(b -> b.field("type", "text"))); - Mapping merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); + Mapping merged = MapperService.mergeMappings( + initMapper, + updatedMapper.mapping(), + randomFrom(MergeReason.MAPPING_UPDATE, MergeReason.MAPPING_AUTO_UPDATE), + Long.MAX_VALUE + ); assertThat(merged.getMeta().get("foo"), equalTo("bar")); updatedMapper = createDocumentMapper(topMapping(b -> b.startObject("_meta").field("foo", "new_bar").endObject())); - merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); + merged = MapperService.mergeMappings( + initMapper, + updatedMapper.mapping(), + randomFrom(MergeReason.MAPPING_UPDATE, MergeReason.MAPPING_AUTO_UPDATE), + Long.MAX_VALUE + ); assertThat(merged.getMeta().get("foo"), equalTo("new_bar")); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index ff2cabde8e0f9..80c074918b06d 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -39,7 +39,11 @@ public void testPreflightUpdateDoesNotChangeMapping() throws Throwable { final MapperService mapperService = createMapperService(mapping(b -> {})); merge(mapperService, MergeReason.MAPPING_AUTO_UPDATE_PREFLIGHT, mapping(b -> createMappingSpecifyingNumberOfFields(b, 1))); assertThat("field was not created by preflight check", mapperService.fieldType("field0"), nullValue()); - merge(mapperService, MergeReason.MAPPING_UPDATE, mapping(b -> createMappingSpecifyingNumberOfFields(b, 1))); + merge( + mapperService, + randomFrom(MergeReason.MAPPING_UPDATE, MergeReason.MAPPING_AUTO_UPDATE), + mapping(b -> createMappingSpecifyingNumberOfFields(b, 1)) + ); assertThat("field was not created by mapping update", mapperService.fieldType("field0"), notNullValue()); } From cb2c260f8616d033261cef8728f52e82ab6a21ce Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Fri, 26 Jan 2024 09:20:50 +0100 Subject: [PATCH 3/4] Use MAPPING_AUTO_UPDATE in another place in PutMappingExecutor.applyRequest --- .../cluster/metadata/MetadataMappingService.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java index 71b0ec9467158..6307ed768e813 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java @@ -148,7 +148,11 @@ private static ClusterState applyRequest( // try and parse it (no need to add it here) so we can bail early in case of parsing exception // first, simulate: just call merge and ignore the result Mapping mapping = mapperService.parseMapping(MapperService.SINGLE_MAPPING_NAME, mappingUpdateSource); - MapperService.mergeMappings(mapperService.documentMapper(), mapping, MergeReason.MAPPING_UPDATE); + MapperService.mergeMappings( + mapperService.documentMapper(), + mapping, + request.autoUpdate() ? MergeReason.MAPPING_AUTO_UPDATE : MergeReason.MAPPING_UPDATE + ); } Metadata.Builder builder = Metadata.builder(metadata); boolean updated = false; From 9a1d504a8aadbf1a189d374aa1ea4fe902706f06 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Fri, 26 Jan 2024 13:36:25 +0100 Subject: [PATCH 4/4] Use MAPPING_AUTO_UPDATE in ParsedDocument.addDynamicMappingsUpdate --- .../java/org/elasticsearch/index/mapper/ParsedDocument.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java b/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java index 014be192a4133..bf540eae5ed49 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java @@ -160,7 +160,7 @@ public void addDynamicMappingsUpdate(Mapping update) { if (dynamicMappingsUpdate == null) { dynamicMappingsUpdate = update; } else { - dynamicMappingsUpdate = dynamicMappingsUpdate.merge(update, MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); + dynamicMappingsUpdate = dynamicMappingsUpdate.merge(update, MergeReason.MAPPING_AUTO_UPDATE, Long.MAX_VALUE); } }