Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate _source.mode in mappings #116689

Merged
merged 21 commits into from
Nov 20, 2024
Merged

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Nov 12, 2024

This change deprecates _source.mode in mappings, replacing it with the index.mapping.source.mode index setting.

@dnhatn dnhatn force-pushed the deprecate-synthetic-source branch 12 times, most recently from c8f9a22 to 63cfc41 Compare November 13, 2024 05:41
@dnhatn dnhatn changed the title Deprecate Deprecate _source.mode in mappings Nov 13, 2024
@dnhatn dnhatn force-pushed the deprecate-synthetic-source branch 2 times, most recently from 496f8d8 to d2b8d68 Compare November 13, 2024 06:31
@dnhatn dnhatn force-pushed the deprecate-synthetic-source branch from d2b8d68 to 1331464 Compare November 13, 2024 16:18
@dnhatn dnhatn added v8.17.0 auto-backport Automatically create backport pull requests when merged :StorageEngine/Mapping The storage related side of mappings >deprecation labels Nov 13, 2024
@elasticsearchmachine
Copy link
Collaborator

Hi @dnhatn, I've created a changelog YAML for you. Note that since this PR is labelled >deprecation, you need to update the changelog YAML to fill out the extended information sections.

@dnhatn dnhatn marked this pull request as ready for review November 13, 2024 16:21
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Nhat, I left questions are the deprecation logic in MetadataCreateIndexService.

I wonder whether the following is perhaps a better mechanism to emit depreciation warning in case _source.mode is used:

Subject: [PATCH] Adjust SyntheticSourceLicenseService 

Allow gold and platinum license to use synthetic source for a limited time.
If the start time of a license if before a cut off date, then gold and platinum licenses will not fallback to stored source if synthetic source is used.
---
Index: server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java
--- a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java	(revision bd091d3d96b33adb4121f980dc4f9f7a2a87b043)
+++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java	(date 1731573325979)
@@ -10,6 +10,8 @@
 package org.elasticsearch.index.mapper;
 
 import org.elasticsearch.common.compress.CompressedXContent;
+import org.elasticsearch.common.logging.DeprecationCategory;
+import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.core.Nullable;
 import org.elasticsearch.index.mapper.MapperService.MergeReason;
@@ -27,6 +29,9 @@
  * Parser for {@link Mapping} provided in {@link CompressedXContent} format
  */
 public final class MappingParser {
+
+    private static final DeprecationLogger DEPRECATION_LOGGER = DeprecationLogger.getLogger(MappingParser.class);
+
     private final Supplier<MappingParserContext> mappingParserContextSupplier;
     private final Supplier<Map<Class<? extends MetadataFieldMapper>, MetadataFieldMapper>> metadataMappersSupplier;
     private final Map<String, MetadataFieldMapper.TypeParser> metadataMapperParsers;
@@ -144,6 +149,14 @@
                 }
                 @SuppressWarnings("unchecked")
                 Map<String, Object> fieldNodeMap = (Map<String, Object>) fieldNode;
+
+                // iirc after parsing fieldNodeMap, keys are removed
+                if (reason == MergeReason.MAPPING_UPDATE && SourceFieldMapper.CONTENT_TYPE.equals(fieldName)) {
+                    if (fieldNodeMap.containsKey("mode")) {
+                        DEPRECATION_LOGGER.critical(DeprecationCategory.MAPPINGS, "mapping_source_mode", SourceFieldMapper.DEPRECATION_WARNING);
+                    }
+                }
+
                 MetadataFieldMapper metadataFieldMapper = typeParser.parse(fieldName, fieldNodeMap, mappingParserContext).build();
                 metadataMappers.put(metadataFieldMapper.getClass(), metadataFieldMapper);
                 assert fieldNodeMap.isEmpty();

I think this also returns warning when mapping gets updated, maybe that isn't too bad? But this should only return a warning when _source.mode attribute is really used.

final IndexSettings indexSettings = mapperService.getIndexSettings();
if (indexSettings != null
&& indexSettings.getIndexVersionCreated().onOrAfter(IndexVersions.DEPRECATE_SOURCE_MODE_MAPPER)
&& SourceFieldMapper.isSynthetic(indexSettings) == false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to check whether index.mapping.source.mode index setting has not be defined here? Other we only check whether this setting has been set to synthetic?

&& indexSettings.getIndexVersionCreated().onOrAfter(IndexVersions.DEPRECATE_SOURCE_MODE_MAPPER)
&& SourceFieldMapper.isSynthetic(indexSettings) == false
&& mapperService.documentMapper() != null
&& mapperService.documentMapper().sourceMapper().isSynthetic()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we check whether index.mapping.source.mode has not be defined, then I think mapperService.documentMapper().sourceMapper().isSynthetic() can return true if index.mode=logsdb|time_series and no _source.mode has been defined?

dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Nov 20, 2024
This change deprecates _source.mode in mappings, replacing it with the
index.mapping.source.mode index setting.
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Nov 20, 2024
This change deprecates _source.mode in mappings, replacing it with the
index.mapping.source.mode index setting.
elasticsearchmachine pushed a commit that referenced this pull request Nov 20, 2024
This change deprecates _source.mode in mappings, replacing it with the
index.mapping.source.mode index setting.
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Nov 20, 2024
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Nov 20, 2024
martijnvg added a commit that referenced this pull request Nov 20, 2024
This reverts commit 0d7b90e, because of bwc testing failures.
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Nov 20, 2024
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Nov 20, 2024
Re-introduce elastic#116689 using cluster features
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Nov 20, 2024
dnhatn added a commit that referenced this pull request Nov 20, 2024
rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Nov 20, 2024
This change deprecates _source.mode in mappings, replacing it with the 
index.mapping.source.mode index setting.
rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Nov 20, 2024
rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Nov 20, 2024
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Nov 21, 2024
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Nov 25, 2024
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Nov 25, 2024
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Nov 25, 2024
dnhatn added a commit that referenced this pull request Nov 25, 2024
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Nov 25, 2024
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Nov 25, 2024
dnhatn added a commit that referenced this pull request Nov 26, 2024
Backport of #116689 to 8.17

This change deprecates _source.mode in mappings, replacing it with
the index.mapping.source.mode index setting.
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
This change deprecates _source.mode in mappings, replacing it with the 
index.mapping.source.mode index setting.
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged >deprecation :StorageEngine/Mapping The storage related side of mappings Team:StorageEngine test-full-bwc Trigger full BWC version matrix tests v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants