Skip to content

Conversation

@cbuescher
Copy link
Member

This change adds a new rolling upgrade test that checks having certain deprecated
index settings present in an N-2 index doesn't prevent the index from being opened
in the current version. The deprecated settings specifically tested here are:

  • index.mapper.dynamic
  • index.max_adjacency_matrix_filters
  • index.force_memory_term_dictionary
  • index.soft_deletes.enabled

Even though that list is not exhaustive of deprecated settings it covers some of the
ones that appeared in several discussions and have been of interest in the past.

@cbuescher cbuescher added :Search Foundations/Search Catch all for Search Foundations v9.0.1 v9.1.0 labels Feb 13, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch label Feb 13, 2025
@cbuescher cbuescher added the >test Issues or PRs that are addressing/adding tests label Feb 13, 2025
assertIndexSetting(index, VERIFIED_BEFORE_CLOSE_SETTING, is(isClosed));
assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true));

var blocks = indexBlocks(index).stream().filter(c -> c.equals(INDEX_WRITE_BLOCK) || c.equals(INDEX_READ_ONLY_BLOCK)).toList();
Copy link
Contributor

Choose a reason for hiding this comment

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

There are parts of code in common between this test class and RollingUpgradeDeprecatedSettingsIT, such as the following code block. We can export methods to the parent class for the duplicated code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I will take a look what makes most sense here.

.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
// add some index settings deprecated in v7
.put(MapperService.INDEX_MAPPER_DYNAMIC_SETTING.getKey(), true)
Copy link
Contributor

@drempapis drempapis Feb 17, 2025

Choose a reason for hiding this comment

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

Is it worth checking the behavior of each deprecated setting? For example, to verify that a new field is added to the mapping when index. mapper.dynamic: true, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, any of these settings shouldn't be used any longer and most of the times we're not even evaluating their setting in production code anywhere (e.g. "index.mapper.dynamic" hasn't been used since #109160 but having the setting in the mapping has created issues in the past). This test merely should assure that we tolerate any of these settings when moving to 9 but not necessarily that any legacy functionality attached to them is still working (which most of the time isn't)

@cbuescher
Copy link
Member Author

@drempapis thanks for the review, I left a note and moved some common test code up to a shared parent class. I didn't opt for making the new test a sub-class of the existing RollingUpgradeLuceneIndexCompatibilityTestCase because I don't want this test to inherit every test of that class and also to avoid coupling test cases too tightly and create too deep inheritance structures in tests, which in my experience can make test readability and maintenance harder in the long run. Let me know if you have anything to add.

@cbuescher
Copy link
Member Author

@drempapis I'm still considering merging this. Any other suggestions or concerns from your end?

Copy link
Contributor

@drempapis drempapis left a comment

Choose a reason for hiding this comment

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

LGTM! thank you @cbuescher for this update

@cbuescher cbuescher added the auto-backport Automatically create backport pull requests when merged label Mar 4, 2025
@cbuescher
Copy link
Member Author

@drempapis thanks for the review

@cbuescher cbuescher merged commit 0f14c4f into elastic:main Mar 4, 2025
17 checks passed
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
9.0

cbuescher added a commit to cbuescher/elasticsearch that referenced this pull request Mar 4, 2025
This change adds a new rolling upgrade test that checks having certain deprecated
index settings present in an N-2 index doesn't prevent the index from being opened
in the current version. The deprecated settings specifically tested here are:

* index.mapper.dynamic
* index.max_adjacency_matrix_filters
* index.force_memory_term_dictionary
* index.soft_deletes.enabled
elasticsearchmachine pushed a commit that referenced this pull request Mar 4, 2025
)

This change adds a new rolling upgrade test that checks having certain deprecated
index settings present in an N-2 index doesn't prevent the index from being opened
in the current version. The deprecated settings specifically tested here are:

* index.mapper.dynamic
* index.max_adjacency_matrix_filters
* index.force_memory_term_dictionary
* index.soft_deletes.enabled
georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Mar 11, 2025
This change adds a new rolling upgrade test that checks having certain deprecated
index settings present in an N-2 index doesn't prevent the index from being opened
in the current version. The deprecated settings specifically tested here are:

* index.mapper.dynamic
* index.max_adjacency_matrix_filters
* index.force_memory_term_dictionary
* index.soft_deletes.enabled
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 :Search Foundations/Search Catch all for Search Foundations Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch >test Issues or PRs that are addressing/adding tests v9.0.1 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants