Skip to content

Conversation

@martijnvg
Copy link
Member

Fail if an index settings provider adds a setting that was added by another index settings provider.

@martijnvg martijnvg added >non-issue :Data Management/Indices APIs APIs to create and manage indices and templates :StorageEngine/Mapping The storage related side of mappings labels Oct 1, 2024
Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Thanks @martijnvg.

@martijnvg martijnvg marked this pull request as ready for review October 9, 2024 14:39
@elasticsearchmachine elasticsearchmachine added Team:Data Management Meta label for data/management team Team:StorageEngine labels Oct 9, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

templateAndRequestSettings,
combinedTemplateMappings
);
validateAdditionalSettings(provider, newAdditionalSettings, additionalIndexSettings);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we validate the newAdditionalSettings against the additionalSettings seen so far and not values previously set by the user for example templateAndRequestSettings. Are these 2 settings sets disjoint ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The newAdditionalSettings variable contains any settings that the current index settings provider might have set. The additionalIndexSettings variable tracks any settings that any index settings provider may have set while looping over the list of index settings providers. The new validation requires that newAdditionalSettings cannot contain any settings that are already defined in additionalIndexSettings. So yes, these two Settings variables should be a disjoint.

Copy link
Contributor

@gmarouli gmarouli left a comment

Choose a reason for hiding this comment

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

LGTM

@martijnvg
Copy link
Member Author

Thanks Mary and Nhat for reviewing!

@martijnvg martijnvg added the auto-backport Automatically create backport pull requests when merged label Oct 11, 2024
@martijnvg martijnvg merged commit bebcaf9 into elastic:main Oct 11, 2024
16 checks passed
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Oct 11, 2024
Fail if an index settings provider adds a setting that was added by another index settings provider.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

elasticsearchmachine pushed a commit that referenced this pull request Oct 11, 2024
Fail if an index settings provider adds a setting that was added by another index settings provider.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
davidkyle pushed a commit to davidkyle/elasticsearch that referenced this pull request Oct 13, 2024
Fail if an index settings provider adds a setting that was added by another index settings provider.
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 :Data Management/Indices APIs APIs to create and manage indices and templates >non-issue :StorageEngine/Mapping The storage related side of mappings Team:Data Management Meta label for data/management team Team:StorageEngine v8.16.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants