-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Adding mappings to data streams #129787
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
Adding mappings to data streams #129787
Conversation
|
Pinging @elastic/es-data-management (Team:Data Management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for storing and merging mappings on data streams by introducing a CompressedXContent mappings object across cluster state serialization, indexing templates, and relevant test coverage. Key changes include:
- Introducing a new randomMappings helper method in test utilities.
- Updating data stream and composable index template classes to include mappings handling.
- Adapting tests and transport actions to factor in mappings for effective index template resolution.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/framework/src/main/java/org/elasticsearch/cluster/metadata/DataStreamTestHelper.java | Added randomMappings() helper for fake mapping generation. |
| server/src/test/java/org/elasticsearch/cluster/metadata/DataStreamTests.java | Updated tests to pass mappings to DataStream instances and verify effective index templates. |
| server/src/test/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplateTests.java | Added tests for merging mappings and settings, including a typo in a test method name. |
| server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateDataStreamService.java | Passed EMPTY_MAPPINGS to newly added DataStream constructor parameter. |
| server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java | Propagated mappings through serialization, effective index template resolution, and xContent output. |
| server/src/main/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplate.java | Introduced mergeMappings() and merged mapping logic for index template updates. |
| TransportVersions.java | Added new transport version for mappings in data streams support. |
| modules/data-streams/src/test/java/org/elasticsearch/datastreams/* | Adjusted tests to include mappings in data stream updates and actions. |
Comments suppressed due to low confidence (1)
server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java:403
- Since getEffectiveIndexTemplate now throws IOException, consider adding or updating the method's Javadoc to clearly document this behavior and provide guidance on exception handling.
public ComposableIndexTemplate getEffectiveIndexTemplate(ProjectMetadata projectMetadata) throws IOException {
server/src/test/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplateTests.java
Outdated
Show resolved
Hide resolved
lukewhiting
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny comment but otherwise looks good 👍🏻
| assertThat(indexTemplate.mergeSettings(dataStreamSettings), equalTo(expectedEffectiveTemplate)); | ||
| } | ||
|
|
||
| public void testMergeEmptyMappingsIntoTemplateWithNonEmptySettings() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a test to make sure the empty into empty short short circuit logic is working or is that overkill?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense -- no harm in adding it.
This adds a mappings CompressedXContent object to each DataStream. This mappings object is serialized into the cluster state, and exposed via a getMappings(), getEffectiveMappings(), and getEffectiveIndexTemplate() methods . Right now it is not used by anything, but will be in subsequent PRs.
The code in this PR is not behind a feature flag, because there is no way for the code to have any user-facing impact, since there is no way yet to actually set a mapping on a data stream. The next PR will add the transport actions for getting and setting the mapping. The final PR will add the rest actions, API spec, and yaml rest tests. That will be behind a feature flag.