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.

Sign up for GitHub

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

Jump to bottom

Turn _source meta fieldmapper's mode attribute into a no-op. #119072

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Turn _source meta fieldmapper's mode attribute into a no-op. #119072

wants to merge 20 commits into from

Conversation

martijnvg
Copy link
Member

Closes #118596

@martijnvg martijnvg mentioned this pull request Dec 24, 2024
Merged
martijnvg
martijnvg commented Jan 14, 2025
View reviewed changes
server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java
@@ -160,13 +164,21 @@ public static class Builder extends MetadataFieldMapper.Builder {
private boolean serializeMode;

private final boolean supportsNonDefaultParameterValues;

public Builder(IndexMode indexMode, final Settings settings, boolean supportsCheckForNonDefaultParams, boolean serializeMode) {
private final boolean sourceModeIsNoop;
Copy link
Member Author

Choose a reason for hiding this comment

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

Another boolean field... but we need to a way to allow indices created before 9.0.0 to support the _source.mode attribute and only from 9.0.0 it will truly be a no-op.

martijnvg
martijnvg commented Jan 14, 2025
View reviewed changes
test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java
@@ -1860,10 +1859,7 @@ public static CreateIndexResponse createIndex(RestClient client, String name, Se

if (settings != null && settings.getAsBoolean(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true) == false) {
expectSoftDeletesWarning(request, name);
} else if (isSyntheticSourceConfiguredInMapping(mapping)
Copy link
Member Author

@martijnvg martijnvg Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Remove logic that allows deprecation warning because of _source.mode mapping attribute usage.
Instead no tests should emit this warning, except tests that specifically test the _source.mode deprecation warning.

martijnvg
martijnvg commented Jan 14, 2025
View reviewed changes
test/yaml-rest-runner/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java
@@ -485,8 +484,6 @@ public void checkWarningHeaders(final List<String> warningHeaders, String testPa
}
}

unexpected.removeIf(s -> s.endsWith(SourceFieldMapper.DEPRECATION_WARNING + "\""));

Copy link
Member Author

Choose a reason for hiding this comment

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

Remove logic that allows deprecation warning because of _source.mode mapping attribute usage.

martijnvg
martijnvg commented Jan 14, 2025
View reviewed changes
x-pack/plugin/logsdb/src/yamlRestTest/resources/rest-api-spec/test/40_source_mode_setting.yml
- match: { test_stored_override.mappings._source.mode: synthetic }

---
override stored to disabled source mode:
Copy link
Member Author

Choose a reason for hiding this comment

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

Tests that both that test overwrite behaviour when source mode index setting and _source.mode mapping attribute are specified are no longer relevant with this change.

martijnvg
martijnvg commented Jan 14, 2025
View reviewed changes
x-pack/plugin/logsdb/src/yamlRestTest/resources/rest-api-spec/test/40_source_mode_setting.yml
modify source mapping from stored to synthetic after index creation:
- do:
warnings:
- "Configuring source mode in mappings is deprecated and will be removed in future versions. Use [index.mapping.source.mode] index setting instead."
Copy link
Member Author

Choose a reason for hiding this comment

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

this diff is messy, but just tests that _source.mode mapping attribute is allowed to be used but is snoop and emits deprecation warning.

@martijnvg
Copy link
Member Author

Note to reviewer:

  • Big part of the change is moving tests to use index.mapping.source.mode index setting instead of _source.mode mapping attribute.
  • Removing tests that specifically tested the presence logic between index.mapping.source.mode index setting and _source.mode mapping attribute.
  • Removing test infra code that silently allowed _source.mode mapping attribute warning.
  • The interesting part of this change is in SourceFieldMapper, where a new field is introduced that allows makes _source.mode attribute a noop for >= 9.0 indices and for <= 9.0 indices still allows the mapping attribute to control source mode.

@martijnvg martijnvg added >breaking :StorageEngine/Mapping The storage related side of mappings labels Jan 15, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@martijnvg martijnvg marked this pull request as ready for review January 15, 2025 12:38
@elasticsearchmachine
Copy link
Collaborator

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

@martijnvg martijnvg changed the title Turn _source meta fieldmapper's mode attribute into a noop. Turn _source meta fieldmapper's mode attribute into a no-op. Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
>breaking :StorageEngine/Mapping The storage related side of mappings Team:StorageEngine v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Turn the _source.mode mapping attribute into a no-op.
2 participants
@martijnvg @elasticsearchmachine