[Sampling] Using index setting providers for data stream setting validation#136214
Conversation
masseyke
added
>bug
:StorageEngine/Data streams
|
Hi @masseyke, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances data stream setting validation by incorporating index setting providers, specifically to resolve failures when applying settings to TSDB (Time Series Data Base) data streams.
- Adds IndexSettingProviders parameter to MetadataDataStreamsService constructor and related methods
- Implements additional settings validation logic that accounts for index setting providers
- Includes new test coverage for TSDB data stream setting validation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| MetadataDataStreamsService.java | Core implementation adding IndexSettingProviders integration and validation logic |
| NodeConstruction.java | Updates service instantiation to include IndexSettingProviders parameter |
| MetadataDataStreamsServiceTests.java | Updates test mocks to include new IndexSettingProviders parameter |
| DataStreamLifecycleWithRetentionWarningsTests.java | Updates test mocks to include new IndexSettingProviders parameter |
| DataStreamSettingsIT.java | Adds integration test for TSDB data stream settings validation |
| 20_metrics_tests.yml | Adds YAML REST test for data stream settings with ILM |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Pinging @elastic/es-data-management (Team:Data Management) |
elasticsearchmachine
added
the
Team:Data Management (obsolete)
…thub.com:masseyke/elasticsearch into data-stream-settings-use-index-setting-providers
dakrone
changed the title
felixbarny
left a comment
felixbarny
left a comment
There was a problem hiding this comment.
LGTM. Just some nits and a suggestion to avoid code duplication, but that can also be done separately from this bug fix in another PR.
server/src/test/java/org/elasticsearch/cluster/metadata/MetadataDataStreamsServiceTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/cluster/metadata/MetadataDataStreamsServiceTests.java
Outdated
Show resolved
Hide resolved
...t/java/org/elasticsearch/cluster/metadata/DataStreamLifecycleWithRetentionWarningsTests.java
Outdated
Show resolved
Hide resolved
| return dataStream.copy().setSettings(mergedDataStreamSettings).build(); | ||
| } | ||
|
|
||
| private Settings addSettingsFromIndexSettingProviders( |
There was a problem hiding this comment.
This is the fifth place where we're essentially repeating the same logic. Is there a way to avoid the duplication by having a common method that we can call in various places?
There was a problem hiding this comment.
Yeah I definitely agree. I'll try to do it in a follow-up PR (I don't want to delay getting this bug fixed b/c I think that'll be a fairly invasive change).
There was a problem hiding this comment.
Hey Keith, friendly reminder to create a follow up PR to address the duplication.
lukewhiting
left a comment
lukewhiting
left a comment
There was a problem hiding this comment.
Nothing to add that other's haven't already spotted but in terms of the function and streams, looks like a sensible change that should fix the bug 👍🏻
Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>
masseyke
added
the
auto-merge-without-approval
💚 Backport successful
|
masseyke
changed the title
This accounts for index setting providers when validating settings. This avoids failures that occur when putting settings on a TSDB data stream.
Closes #136166