- 
                Notifications
    
You must be signed in to change notification settings  - Fork 25.6k
 
Taking additional settings providers into account for data stream effective settings #137407
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
base: main
Are you sure you want to change the base?
Taking additional settings providers into account for data stream effective settings #137407
Conversation
| 
           Hi @masseyke, I've created a changelog YAML for you.  | 
    
…:masseyke/elasticsearch into data-stream-settings-additional-settings
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 enhances data stream effective settings calculation by incorporating additional settings from index setting providers. Previously, getEffectiveSettings() only considered template and data stream settings, but now it accepts a function parameter to apply implicit settings from index setting providers.
Key changes:
- Modified 
DataStream.getEffectiveSettings()to accept aFunction<Settings, Settings>parameter for applying additional settings - Made 
MetadataDataStreamsService.addSettingsFromIndexSettingProviders()public and removedthrows IOExceptiondeclaration - Updated all callers of 
getEffectiveSettings()to pass the appropriate function, typically invokingaddSettingsFromIndexSettingProviders() 
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description | 
|---|---|
| server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java | Modified getEffectiveSettings() signature to accept a function parameter for applying implicit settings | 
| server/src/main/java/org/elasticsearch/cluster/metadata/MetadataDataStreamsService.java | Changed addSettingsFromIndexSettingProviders() visibility to public and removed throws IOException | 
| server/src/test/java/org/elasticsearch/cluster/metadata/DataStreamTests.java | Updated test calls to getEffectiveSettings() with identity function settings -> settings | 
| server/src/test/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverServiceTests.java | Updated test call to getEffectiveSettings() with identity function | 
| modules/data-streams/src/main/java/org/elasticsearch/datastreams/action/TransportGetDataStreamsAction.java | Added dependencies and updated to call getEffectiveSettings() with index setting providers function | 
| modules/data-streams/src/main/java/org/elasticsearch/datastreams/action/TransportGetDataStreamSettingsAction.java | Added dependencies and updated to call getEffectiveSettings() with index setting providers function | 
| modules/data-streams/src/main/java/org/elasticsearch/datastreams/action/TransportUpdateDataStreamSettingsAction.java | Added dependencies and updated to call getEffectiveSettings() with index setting providers function | 
| modules/data-streams/src/test/java/org/elasticsearch/datastreams/action/TransportGetDataStreamsActionTests.java | Added mock setup for MetadataDataStreamsService and IndicesService dependencies | 
| docs/changelog/137407.yaml | Added changelog entry documenting the bug fix | 
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
        
          
                ...src/main/java/org/elasticsearch/datastreams/action/TransportGetDataStreamSettingsAction.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                .../main/java/org/elasticsearch/datastreams/action/TransportUpdateDataStreamSettingsAction.java
          
            Show resolved
            Hide resolved
        
              
          
                ...treams/src/main/java/org/elasticsearch/datastreams/action/TransportGetDataStreamsAction.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      …m hsa not rolled over yet
| 
           Hi @masseyke, I've updated the changelog YAML for you.  | 
    
…:masseyke/elasticsearch into data-stream-settings-additional-settings
| 
           Pinging @elastic/es-data-management (Team:Data Management)  | 
    
The method used to get effective settings for a data stream did not take settings from IndexSettingsProviders into account. This caused the get data stream mappings API to crash for time_series data streams since settings were missing.
This PR moves
getEffectiveSettingsfrom DataStream to MetadataDataStreamsService, and adds the implicit settings from IndexSettingsProviders to the effective settings.Closes #137381