Configure IndexSearcher.maxClauseCount() based on Node characteristics #81525

romseygeek · 2021-12-08T13:54:07Z

This commit deprecates the indices.query.bool.max_clause_count node setting,
and instead configures the maximum clause count for lucene based on the available
heap and the size of the thread pool.

Closes #46433

elasticmachine · 2021-12-08T13:54:11Z

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

romseygeek · 2021-12-08T13:55:17Z

I'm not totally happy with the testing on this yet, and it will need some documentation as well, but it's good enough for a preliminary review I think.

jpountz · 2021-12-09T16:31:22Z

server/src/main/java/org/elasticsearch/search/SearchUtils.java

+    public static void configureMaxClauses(ThreadPool threadPool) {
+        int searchThreadPoolSize = threadPool.info(ThreadPool.Names.SEARCH).getMax();
+        long heapSize = JvmStats.jvmStats().getMem().getHeapMax().getGb();
+        configureMaxClauses(searchThreadPoolSize, heapSize);


What happens if you start Elasticsearch with 512MB of heap, does it mean you get zero max clauses?

If heapSize is zero then we stick with the default value from lucene, currently 1024. But maybe we should stick with the current ES default of 4096 instead?

Have updated this so that we will always return a minimum of 4096, or larger if the heap/cpu size permits.

Would this work if we used the heap size in GB as a fractional number instead?

jpountz · 2021-12-09T16:32:07Z

server/src/main/java/org/elasticsearch/search/SearchUtils.java

+            return;     // If we don't know how to size things, keep the lucene default
+        }
+
+        int maxClauseCount = Math.toIntExact(heapInGb * 65_536 / threadPoolSize);


Let's add some explanation as to where this 65,536 number comes from?

jpountz · 2021-12-09T16:33:40Z

...a/org/elasticsearch/search/aggregations/bucket/adjacency/AdjacencyMatrixAggregatorTests.java

-                    + "This limit can be set by changing the ["
-                    + SearchModule.INDICES_MAX_CLAUSE_COUNT_SETTING.getKey()
-                    + "] setting."
+                "Number of filters is too large, must be less than or equal to: [" + maxFilters + "] but was [" + maxFiltersPlusOne + "]."


Let's add guidance as to how to address this error? E.g. can we check if the user overrode the size of the search thread and suggest undoing the change? Or otherwise recommend scaling up or avdoiding running queries with such large numbers of clauses?

jpountz · 2021-12-09T16:34:35Z

server/src/main/java/org/elasticsearch/search/SearchModule.java

@@ -268,7 +267,8 @@
        4096,
        1,
        Integer.MAX_VALUE,
-        Setting.Property.NodeScope
+        Setting.Property.NodeScope,
+        Setting.Property.DeprecatedWarning


When would we raise a deprecation warning for node settings, only on startup?

Have confirmed with @pgomulka that this will only be emitted on startup, yes.

romseygeek · 2021-12-13T12:50:37Z

I've reworked things a bit: the utils class now only calculates the max clauses value, it is set directly in the node startup code, and we use the current default value (4096) as a minimum so that we won't inadvertently limit users query sizes when then upgrade.

romseygeek · 2021-12-13T14:25:09Z

Interesting, this causes QueryStringIT#testLimitOnExpandedFieldsButIgnoreUnmappedFields to fail on CI as we run into a too many fields exception, but this doesn't reproduce locally because my machine has more memory than the CI worker. I'll rework the test to be a unit test for the query string query builder, rather than an integration test.

romseygeek · 2021-12-14T10:11:26Z

@elasticmachine run elasticsearch-ci/part-2

jpountz · 2021-12-14T13:21:29Z

docs/reference/migration/migrate_8_0/cluster-node-setting-changes.asciidoc

-of clauses of a single `bool` query. It now applies to the total number of
-clauses of the rewritten query. To reduce chances of breaks, its
-default value has been bumped from 1024 to 4096.
+Elasticsearch will now dynamically set the maximum number of allowed clauses


Maybe we need a word about the fact that Elasticsearch now checks the number of clauses across the entire query rather than on a per-booleanquery basis? So that anyone who has used the hack that consists of splitting boolean queries knows that they can undo it.

jpountz · 2021-12-14T13:21:46Z

docs/reference/migration/migrate_8_0/cluster-node-setting-changes.asciidoc

+Elasticsearch will now dynamically set the maximum number of allowed clauses
+in a query, using a heuristic based on the size of the search thread pool and
+the size of the heap allocated to the JVM. This limit has a minimum value of
+4096 (the previous default) and will in most cases be larger (for example,


1024 was the previous value, not 4096?

jpountz · 2021-12-14T13:22:39Z

docs/reference/migration/migrate_8_0/cluster-node-setting-changes.asciidoc

-you might need to increase it further. 
+Queries with many clauses should be avoided whenever possible.
+If you previously bumped this setting to accommodate heavy queries,
+you might need to increase the amount of memory available to elasticsearch,


Suggested change

you might need to increase the amount of memory available to elasticsearch,

you might need to increase the amount of memory available to Elasticsearch,

jpountz · 2021-12-14T13:27:17Z

server/src/main/java/org/elasticsearch/search/SearchUtils.java

+    public static void configureMaxClauses(ThreadPool threadPool) {
+        int searchThreadPoolSize = threadPool.info(ThreadPool.Names.SEARCH).getMax();
+        long heapSize = JvmStats.jvmStats().getMem().getHeapMax().getGb();
+        configureMaxClauses(searchThreadPoolSize, heapSize);


Would this work if we used the heap size in GB as a fractional number instead?

jpountz · 2021-12-14T13:29:43Z

docs/reference/migration/migrate_8_0/cluster-node-setting-changes.asciidoc

-default value has been bumped from 1024 to 4096.
+Elasticsearch will now dynamically set the maximum number of allowed clauses
+in a query, using a heuristic based on the size of the search thread pool and
+the size of the heap allocated to the JVM. This limit has a minimum value of


I'm tempted to clarify that this number increases with heap size and decreases with the size of the threadpool, wdyt?

jpountz · 2021-12-14T13:36:39Z

server/src/test/java/org/elasticsearch/search/SearchUtilsTests.java

+        assertEquals(4096, SearchUtils.calculateMaxClauseValue(4, 0));
+
+        // Number of processors not available
+        assertEquals(4096, SearchUtils.calculateMaxClauseValue(-1, 1));


is it actually something that can happen? While the number of cores can be unknown, we are actually using the threadpool size, which should always be defined?

I'm fairly sure that this would always be set, but put this in as a safety net. But maybe it should be replaced with an assertion that the thread pool size > 1

romseygeek · 2021-12-14T14:53:26Z

Would this work if we used the heap size in GB as a fractional number instead?

I'm not sure what you mean here?

jpountz · 2021-12-14T18:02:33Z

Would this work if we used the heap size in GB as a fractional number instead?

I'm not sure what you mean here?

We currently get the Java heap in GB as a long. A side-effect of this is that the formula always returns 0 as the maximum number of clauses if the heap is less than 1GB since the amount of memory allocated to the JVM is rounded down. Si I wonder if we should use a fractional representation of the heap size instead, e.g. 0.5 if the JVM is given 512MB of heap size? This way, we might also not need to resort to taking the max of the return value and 4096, it should better scale to small heaps?

jpountz · 2021-12-16T17:14:24Z

docs/reference/migration/migrate_8_0/cluster-node-setting-changes.asciidoc

+or to reduce the size of your search thread pool so that more memory is
+available to each concurrent search.
+
+In previous versions of lucene you could get around this limit by nesting


Suggested change

In previous versions of lucene you could get around this limit by nesting

In previous versions of Lucene you could get around this limit by nesting

jpountz · 2021-12-16T17:15:38Z

...internalClusterTest/java/org/elasticsearch/search/aggregations/bucket/AdjacencyMatrixIT.java

@@ -280,7 +279,7 @@ public void testTooLargeMatrix() throws Exception {

        // Create more filters than is permitted by Lucene Bool clause settings.
        MapBuilder filtersMap = new MapBuilder();
-        int maxFilters = SearchModule.INDICES_MAX_CLAUSE_COUNT_SETTING.get(Settings.EMPTY);
+        int maxFilters = IndexSearcher.getMaxClauseCount();


This number might be super high now, how slow is the test? Maybe we need to set an artificially low limit for this test to keep it reasonable?

jpountz · 2021-12-16T17:19:01Z

...rg/elasticsearch/search/aggregations/bucket/adjacency/AdjacencyMatrixAggregationBuilder.java

-                    + SearchModule.INDICES_MAX_CLAUSE_COUNT_SETTING.getKey()
-                    + "] setting."
+                    + "].  "
+                    + "You can increase this limit by scaling up your java heap or number of CPUs"


scaling up the number of CPUs would increase the size of the search threadpool and reduce the number of allowed clauses, so maybe just mention memory?

elasticsearchmachine · 2021-12-17T11:38:37Z

💚 Backport successful

Status	Branch	Result
✅	8.0

elastic#81525) This commit deprecates the indices.query.bool.max_clause_count node setting, and instead configures the maximum clause count for lucene based on the available heap and the size of the thread pool. Closes elastic#46433

#81525) (#81850) This commit deprecates the indices.query.bool.max_clause_count node setting, and instead configures the maximum clause count for lucene based on the available heap and the size of the thread pool. Closes #46433

romseygeek added 2 commits December 8, 2021 13:50

Configure max clause size based on search pool size and max heap

599c297

Merge remote-tracking branch 'origin/master' into search/max-clauses

7b7ae11

romseygeek added :Search/Search Search-related issues that do not fall into other categories >deprecation v8.0.0 v8.1.0 labels Dec 8, 2021

romseygeek self-assigned this Dec 8, 2021

elasticmachine added the Team:Search Meta label for search team label Dec 8, 2021

spotless

fa76baa

jpountz reviewed Dec 9, 2021

View reviewed changes

romseygeek added 2 commits December 13, 2021 10:59

Merge remote-tracking branch 'origin/master' into search/max-clauses

47e4949

reworking

6f67b60

romseygeek added 2 commits December 13, 2021 12:55

spotless

ee6b47f

test message

949b32f

romseygeek added 2 commits December 13, 2021 15:06

Move test that unmapped fields do not get resolved to unit test

2627e4a

Merge remote-tracking branch 'origin/master' into search/max-clauses

a053462

romseygeek added 2 commits December 14, 2021 11:52

Add entry to breaking changes

691e7a9

Merge remote-tracking branch 'origin/master' into search/max-clauses

51976f2

romseygeek requested a review from jpountz December 14, 2021 12:15

jpountz reviewed Dec 14, 2021

View reviewed changes

romseygeek added 2 commits December 15, 2021 10:38

Lower min value to 1024; use fractional GB value

0c56f81

spotless

8990bb2

jpountz approved these changes Dec 16, 2021

View reviewed changes

romseygeek added 2 commits December 17, 2021 09:59

Merge remote-tracking branch 'origin/master' into search/max-clauses

baf1f4a

deef

c34cdfe

romseygeek added the auto-backport-and-merge label Dec 17, 2021

romseygeek merged commit f0bf6f5 into elastic:master Dec 17, 2021

romseygeek deleted the search/max-clauses branch December 17, 2021 11:37

romseygeek mentioned this pull request Dec 17, 2021

[8.0] Configure IndexSearcher.maxClauseCount() based on Node characteristics (#81525) #81850

Merged

romseygeek mentioned this pull request Dec 20, 2021

Expose node maximum query clause count in stats #81913

Closed

pugnascotia added v8.0.0-rc2 and removed v8.0.0 labels Feb 1, 2022

nastasha-solomon mentioned this pull request Jun 30, 2022

[DOCS] Docs for the indices.query.bool.max_clause_count setting might need to be updated #88197

Closed

nickcanz mentioned this pull request Nov 22, 2022

[docs] Update search-settings documentation to reflect the fact that the indices.query.bool.max_clause_count setting has been deprecated #91811

Merged

sarayourfriend mentioned this pull request Jun 5, 2023

Sensitive terms list produces too many clauses for create filtered index call WordPress/openverse#2328

Closed

DaveCTurner mentioned this pull request Jun 28, 2023

Improve indices.query.bool.max_clause_count in 7.x #97180

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure IndexSearcher.maxClauseCount() based on Node characteristics #81525

Configure IndexSearcher.maxClauseCount() based on Node characteristics #81525

romseygeek commented Dec 8, 2021

elasticmachine commented Dec 8, 2021

romseygeek commented Dec 8, 2021

jpountz Dec 9, 2021

romseygeek Dec 13, 2021

romseygeek Dec 13, 2021

jpountz Dec 14, 2021

jpountz Dec 9, 2021

jpountz Dec 9, 2021

jpountz Dec 9, 2021

romseygeek Dec 13, 2021

romseygeek commented Dec 13, 2021

romseygeek commented Dec 13, 2021

romseygeek commented Dec 14, 2021

jpountz Dec 14, 2021 •

edited

Loading

jpountz Dec 14, 2021

jpountz Dec 14, 2021

jpountz Dec 14, 2021

jpountz Dec 14, 2021

jpountz Dec 14, 2021

romseygeek Dec 14, 2021

romseygeek commented Dec 14, 2021

jpountz commented Dec 14, 2021

jpountz Dec 16, 2021

jpountz Dec 16, 2021

jpountz Dec 16, 2021

elasticsearchmachine commented Dec 17, 2021

	you might need to increase the amount of memory available to elasticsearch,
	you might need to increase the amount of memory available to Elasticsearch,

	In previous versions of lucene you could get around this limit by nesting
	In previous versions of Lucene you could get around this limit by nesting

Configure IndexSearcher.maxClauseCount() based on Node characteristics #81525

Configure IndexSearcher.maxClauseCount() based on Node characteristics #81525

Conversation

romseygeek commented Dec 8, 2021

elasticmachine commented Dec 8, 2021

romseygeek commented Dec 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romseygeek commented Dec 13, 2021

romseygeek commented Dec 13, 2021

romseygeek commented Dec 14, 2021

jpountz Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romseygeek commented Dec 14, 2021

jpountz commented Dec 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Dec 17, 2021

💚 Backport successful

jpountz Dec 14, 2021 •

edited

Loading