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.

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

Fix skip caching factor with indices.queries.cache.all_segments #85510

Merged
merged 5 commits into from Apr 1, 2022

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Mar 30, 2022

Spin off #85423

The indices.queries.cache.all_segments setting should not be used in production environments. And we should also use the default skipCachingFactor (i.e, 10) for LRUQueryCache when that setting is enabled.

@dnhatn dnhatn added >bug :Search/Search Search-related issues that do not fall into other categories auto-backport-and-merge Automatically create backport pull requests and merge when ready v7.17.3 v8.1.3 v8.3.0 v8.2.1 labels Mar 30, 2022
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Mar 30, 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.

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 looks good to me. I left a small comment for you to consider, but feel free to merge!

@@ -345,6 +353,79 @@ public void testStatsOnEviction() throws IOException {
cache.close(); // this triggers some assertions
}

public void testSkipCachingFactorAtLeast10() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Small comment -- to me this test doesn't add much except guard against someone accidentally changing the skip factor. I think it'd accomplish the same to add a code comment like "the skipFactor must be 10 here to match the default in Lucene's LRUQueryCache".

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.

Agreed with Julie's feedback, otherwise LGTM.

@dnhatn
Copy link
Member Author

dnhatn commented Apr 1, 2022

@jtibshirani @jpountz Thanks for review.

@dnhatn dnhatn merged commit 4a3f9bb into elastic:master Apr 1, 2022
@dnhatn dnhatn deleted the fix_skip_caching_factor branch April 1, 2022 19:09
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Apr 1, 2022
…tic#85510)

The indices.queries.cache.all_segments setting should not be used 
in production environments. And we should also use the default 
skipCachingFactor (i.e, 10) for when that setting is enabled.
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Apr 1, 2022
…tic#85510)

The indices.queries.cache.all_segments setting should not be used 
in production environments. And we should also use the default 
skipCachingFactor (i.e, 10) for when that setting is enabled.
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Apr 1, 2022
…tic#85510)

The indices.queries.cache.all_segments setting should not be used 
in production environments. And we should also use the default 
skipCachingFactor (i.e, 10) for when that setting is enabled.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
7.17
8.1
8.2

elasticsearchmachine pushed a commit that referenced this pull request Apr 1, 2022
…) (#85646)

The indices.queries.cache.all_segments setting should not be used 
in production environments. And we should also use the default 
skipCachingFactor (i.e, 10) for when that setting is enabled.
elasticsearchmachine pushed a commit that referenced this pull request Apr 1, 2022
…) (#85647)

The indices.queries.cache.all_segments setting should not be used 
in production environments. And we should also use the default 
skipCachingFactor (i.e, 10) for when that setting is enabled.
elasticsearchmachine pushed a commit that referenced this pull request Apr 1, 2022
…) (#85648)

The indices.queries.cache.all_segments setting should not be used 
in production environments. And we should also use the default 
skipCachingFactor (i.e, 10) for when that setting is enabled.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport-and-merge Automatically create backport pull requests and merge when ready >bug :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v7.17.3 v8.1.3 v8.2.1 v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants