Skip to content

Conversation

@jpountz
Copy link
Contributor

@jpountz jpountz commented Jul 13, 2021

In the upcoming Lucene 9 release, indices.query.bool.max_clause_count is
going to apply to the entire query tree rather than per bool query. In order
to avoid breaks, the limit has been bumped from 1024 to 4096.

The semantics will effectively change when we upgrade to Lucene 9, this PR
is only about agreeing on a migration strategy and documenting this change.

To avoid further breaks, I am leaning towards keeping the current setting name
even though it contains bool. I believe that it still makes sense given that
bool queries are typically the main contributors to high numbers of clauses.

In the upcoming Lucene 9 release, `indices.query.bool.max_clause_count` is
going to apply to the entire query tree rather than per `bool` query. In order
to avoid breaks, the limit has been bumped from 1024 to 4096.

The semantics will effectively change when we upgrade to Lucene 9, this PR
is only about agreeing on a migration strategy and documenting this change.

To avoid further breaks, I am leaning towards keeping the current setting name
even though it contains `bool`. I believe that it still makes sense given that
`bool` queries are typically the main contributors to high numbers of clauses.
@jpountz jpountz added >enhancement >breaking :Search/Search Search-related issues that do not fall into other categories labels Jul 13, 2021
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jul 13, 2021
@elasticmachine
Copy link
Collaborator

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

@jpountz jpountz requested review from jrodewig and romseygeek July 13, 2021 13:03
@jpountz jpountz mentioned this pull request Jul 13, 2021
16 tasks
Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

Looks good! I have a couple of questions.

public class SearchModule {
public static final Setting<Integer> INDICES_MAX_CLAUSE_COUNT_SETTING = Setting.intSetting("indices.query.bool.max_clause_count",
1024, 1, Integer.MAX_VALUE, Setting.Property.NodeScope);
2048, 1, Integer.MAX_VALUE, Setting.Property.NodeScope);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be 4096?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back and forth between 2048 and 4096, thanks for catching!

from becoming too large, and taking up too much CPU and memory. In case you're considering
increasing this setting, make sure you've exhausted all other options to avoid having to do this.
Higher values can lead to performance degradations and memory issues, especially in clusters with
a high load or few resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's worth mentioning terms queries and index_prefix settings here as well?

@jpountz jpountz requested a review from romseygeek July 13, 2021 14:59
Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@jpountz jpountz changed the title Changed semantics of indices.query.bool.max_clause_count. indices.query.bool.max_clause_count now limits all query clauses Jul 21, 2021
@jpountz jpountz merged commit feb6620 into elastic:master Jul 21, 2021
@jpountz jpountz deleted the max_clause_count branch July 21, 2021 10:16
jpountz added a commit to jpountz/elasticsearch that referenced this pull request Jul 22, 2021
They are due to changes to `indices.query.bool.max_clause_count` that got pushed
in elastic#75297.

Closes elastic#75591
Closes elastic#75592
jpountz added a commit that referenced this pull request Jul 22, 2021
They are due to changes to `indices.query.bool.max_clause_count` that got pushed
in #75297.

Closes #75591
Closes #75592
ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request Jul 30, 2021
…lastic#75297)

In the upcoming Lucene 9 release, `indices.query.bool.max_clause_count` is
going to apply to the entire query tree rather than per `bool` query. In order
to avoid breaks, the limit has been bumped from 1024 to 4096.

The semantics will effectively change when we upgrade to Lucene 9, this PR
is only about agreeing on a migration strategy and documenting this change.

To avoid further breaks, I am leaning towards keeping the current setting name
even though it contains `bool`. I believe that it still makes sense given that
`bool` queries are typically the main contributors to high numbers of clauses.

Co-authored-by: James Rodewig <[email protected]>
ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request Jul 30, 2021
They are due to changes to `indices.query.bool.max_clause_count` that got pushed
in elastic#75297.

Closes elastic#75591
Closes elastic#75592
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>breaking >enhancement :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants