Skip to content

Remove IndexSettingDeprecatedInV7AndRemovedInV8 removal TODOs #119842

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove IndexSettingDeprecatedInV7AndRemovedInV8 removal TODOs #119842

wants to merge 2 commits into from

Conversation

cbuescher
Copy link
Member

Index settings using this property have been marked for removal in v9,
but we still need to support reading indices from v7 now (via the new
N-2 read-only support policy), so we need to keep these settings around as well.

@cbuescher
Remove IndexSettingDeprecatedInV7AndRemovedInV8 removal TODOs
e651a01 
Index settings using this property have been marked for removal in v9,
but we still need to support reading indices from v7 now (via the new
N-2 read-only support policy), so we need to keep these settings around
as well.
@cbuescher cbuescher added >non-issue :Search/Search Search-related issues that do not fall into other categories :StorageEngine/Mapping The storage related side of mappings v9.0.0 labels Jan 9, 2025
@cbuescher cbuescher requested review from javanna and drempapis January 9, 2025 12:03
@cbuescher cbuescher requested a review from a team as a code owner January 9, 2025 12:03
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@elasticsearchmachine
Copy link
Collaborator

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

ldematte
ldematte reviewed Jan 9, 2025
View reviewed changes
Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

Since you are there, what about replacing the TODOs with @UpdateForV10 annotations, so they will be easier to find?

alexey-ivanov-es
alexey-ivanov-es reviewed Jan 9, 2025
View reviewed changes
server/src/main/java/org/elasticsearch/common/settings/Setting.java Outdated
*/
@UpdateForV9(owner = UpdateForV9.Owner.CORE_INFRA) // introduce IndexSettingDeprecatedInV8AndRemovedInV9 to replace this constant
@UpdateForV9(owner = UpdateForV9.Owner.CORE_INFRA) // introduce IndexSettingDeprecatedInV8AndRemovedInV9.
Copy link
Contributor

Choose a reason for hiding this comment

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

We have validation checking that a setting marked with this constant is used in an index created before v8. When introducing IndexSettingDeprecatedInV8AndRemovedInV9 (which will be needed, at least for the index.merge.policy.max_merge_at_once_explicit setting), which IndexVersion should be checked in this case? IndexVersions.UPGRADE_TO_LUCENE_10_0_0?

Copy link
Member

Choose a reason for hiding this comment

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

that's a good q. Technically, upgrade to lucene 10 is not yet 9.0. I would rather think that we should refer to a future index version that's tied to the actual removal of the index setting for v9? Or has that already happened?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this hasn't happened yet, and I'm not sure when or which team will handle it. I just found a deprecated index setting with a comment referencing IndexSettingDeprecatedInV8AndRemovedInV9. Adding those checks when the settings are actually removed sounds good - we'll just need a notification when that's planned

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added IndexSettingDeprecatedInV8AndRemovedInV9: #120334

@javanna javanna added :Search Foundations/Search Catch all for Search Foundations and removed :Search/Search Search-related issues that do not fall into other categories labels Jan 23, 2025
@elasticsearchmachine elasticsearchmachine added Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch and removed Team:Search Meta label for search team labels Jan 23, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldematte ldematte ldematte left review comments

@alexey-ivanov-es alexey-ivanov-es alexey-ivanov-es left review comments

@javanna javanna Awaiting requested review from javanna

@drempapis drempapis Awaiting requested review from drempapis

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
>non-issue :Search Foundations/Search Catch all for Search Foundations :StorageEngine/Mapping The storage related side of mappings Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch Team:StorageEngine v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cbuescher @elasticsearchmachine @javanna @ldematte @alexey-ivanov-es