Skip to content
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

Deprecate [indices.queries.cache.all_segments] setting #85423

Closed
wants to merge 15 commits into from
Closed

Deprecate [indices.queries.cache.all_segments] setting #85423

wants to merge 15 commits into from

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Mar 28, 2022 •
edited

Theindices.queries.cache.all_segments setting should not be used in production environments. This change deprecates that setting.

@dnhatn dnhatn added >bug :Search/Search Search-related issues that do not fall into other categories v8.2.0 v8.1.2 v7.17.3 labels Mar 28, 2022
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Mar 28, 2022
@elasticmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Hi @dnhatn, I've created a changelog YAML for you.

jpountz
jpountz approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Thanks @dnhatn. I am not familiar enough with the settings infrastructure anymore to know whether this is the best way to do this, but I agree we should warn users when they do this.

@dnhatn
Copy link
Member Author

dnhatn commented Mar 29, 2022

@jpountz Thanks for review. Are we okay to remove that setting and register it in a test plugin? I was worried that is a breaking change.

jtibshirani
jtibshirani approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

This makes sense to me too. I'm wondering if we should mention "the setting will be removed in a future version", and if so we could make this a deprecation warning?

jtibshirani
jtibshirani reviewed Mar 29, 2022
View reviewed changes
Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

Expanding on my last comment -- what if we marked this setting as deprecated too, so we could remove it entirely in a future version? I think we do that by adding Property.DEPRECATED to the setting.

server/src/main/java/org/elasticsearch/indices/IndicesService.java Outdated
@@ -796,7 +796,7 @@ public synchronized void verifyIndexMetadata(IndexMetadata metadata, IndexMetada
IndicesFieldDataCache indicesFieldDataCache = new IndicesFieldDataCache(settings, new IndexFieldDataCache.Listener() {
});
closeables.add(indicesFieldDataCache);
IndicesQueryCache indicesQueryCache = new IndicesQueryCache(settings);
IndicesQueryCache indicesQueryCache = new IndicesQueryCache(Settings.EMPTY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to make this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, to avoid many warning/deprecation logs. The cache here is just a place holder.

@dnhatn
Copy link
Member Author

dnhatn commented Mar 29, 2022

what if we marked this setting as deprecated too, so we could remove it entirely in a future version

Yes, but that would require larger changes (i.e., handling deprecation logs in tests). I will make it in a follow-up as I prefer to have this warning in 8.2.0.

jtibshirani
jtibshirani approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

Thanks for the explanations, sounds good to get this warning into 8.2.

@dnhatn dnhatn changed the title Warn if use [indices.queries.cache.all_segments] setting Deprecate [indices.queries.cache.all_segments] setting Mar 30, 2022
@dnhatn
Copy link
Member Author

dnhatn commented Mar 30, 2022

@jtibshirani We have a nice infra to verify deprecated settings. I've updated this PR to deprecate the setting instead.

@dnhatn dnhatn mentioned this pull request Mar 30, 2022
Merged
@dnhatn dnhatn added >deprecation and removed >bug labels Apr 1, 2022
@elasticsearchmachine
Copy link
Collaborator

Hi @dnhatn, I've updated the changelog YAML for you. Note that since this PR is labelled >deprecation, you need to update the changelog YAML to fill out the extended information sections.

@elasticsearchmachine elasticsearchmachine changed the base branch from master to main July 22, 2022 23:08
@mark-vieira mark-vieira added v8.5.0 and removed v8.4.0 labels Jul 27, 2022
@csoulios csoulios added v8.6.0 and removed v8.5.0 labels Sep 21, 2022
@dnhatn dnhatn removed :Search/Search Search-related issues that do not fall into other categories >deprecation Team:Search Meta label for search team v8.1.2 v7.17.3 v8.6.0 labels Nov 3, 2022
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Nov 3, 2022
@dnhatn
Copy link
Member Author

dnhatn commented Nov 3, 2022

We can leave the test setting as is.

@dnhatn dnhatn closed this Nov 3, 2022
@dnhatn dnhatn deleted the skip_cache_factor branch November 3, 2022 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpountz jpountz jpountz approved these changes

@jtibshirani jtibshirani jtibshirani approved these changes

Assignees
No one assigned
Labels
needs:triage Requires assignment of a team area label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@dnhatn @elasticmachine @elasticsearchmachine @jpountz @jtibshirani @craigtaverner @csoulios @mark-vieira @salvatore-campagna