Set INDICES_MAX_CLAUSE_COUNT dynamically #13568

harshavamsi · 2024-05-06T22:36:22Z

Description

Changes the INDICES_MAX_CLAUSE_COUNT_SETTING to be dynamic instead of being static.

This PR is virtually identical to #1527, but there was earlier push back around making this an updateable dynamic setting due to the assumption that users would abuse this setting rather than optimize their underlying query. Lucene has since updated the way the clause counts work and this is now enforced at the indexSearcher level -- https://issues.apache.org/jira/browse/LUCENE-8811.

More info can be found in #12549 around the motivation for this change, but I ended up raising a PR to make a simple dynamic setting.

Related Issues

Resolves #1526 #12549

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

github-actions · 2024-05-06T22:47:45Z

❌ Gradle check result for dd140e2: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

jainankitk · 2024-05-06T23:38:18Z

server/src/main/java/org/opensearch/search/SearchService.java

+        IndexSearcher.setMaxClauseCount(INDICES_MAX_CLAUSE_COUNT_SETTING.get(settings));
+        clusterService.getClusterSettings().addSettingsUpdateConsumer(INDICES_MAX_CLAUSE_COUNT_SETTING, IndexSearcher::setMaxClauseCount);


Can we add local settings consumer? That can further delegate the new value to IndexSearcher.setMaxClauseCount. The local attribute can be used as is within checkForTooManyFields method above.

thx @jainankitk for the feedback! I'm wondering what the use-case for the consumer might be? If we're only setting the searcher clause count and feeding it to the settings consumer wouldn't that be enough?

What is the purpose of below expression? Is it possible to override this setting at index or request level? If not, we can add getter in search using the consumer mentioned above.

Integer limit = SearchService.INDICES_MAX_CLAUSE_COUNT_SETTING.get(context.getIndexSettings().getSettings());

It registers a consumer for and adds it to the cluster settings. The consumer is the IndexSearcher.setMaxClauseCount.

Is it possible to override this setting at index or request level?

It is only possible to override this setting at the cluster level since we'd want all indices to have this effect.

It is only possible to override this setting at the cluster level since we'd want all indices to have this effect.

In that case, why not change to below:

this.setMaxClauseCount(INDICES_MAX_CLAUSE_COUNT_SETTING.get(settings)); clusterService.getClusterSettings().addSettingsUpdateConsumer(INDICES_MAX_CLAUSE_COUNT_SETTING, this::setMaxClauseCount);

private void setMaxClauseCount(int maxClauseCount) { this.maxClauseCount = maxClauseCount; IndexSearcher.setMaxClauseCount(maxClauseCount); } // Used in SearchModule above public int getMaxClauseCount() { return this.maxClauseCount; }

I made it a static method and now using getter and setter.

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

github-actions · 2024-05-07T00:28:55Z

❌ Gradle check result for 28d5525: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

jainankitk · 2024-05-15T00:03:21Z

server/src/main/java/org/opensearch/index/search/QueryParserHelper.java

@@ -177,7 +177,7 @@ static Map<String, Float> resolveMappingField(
    }

    static void checkForTooManyFields(int numberOfFields, QueryShardContext context, @Nullable String inputPattern) {
-        Integer limit = SearchModule.INDICES_MAX_CLAUSE_COUNT_SETTING.get(context.getIndexSettings().getSettings());
+        Integer limit = SearchService.INDICES_MAX_CLAUSE_COUNT_SETTING.get(context.getIndexSettings().getSettings());


Can be changed to below:

Integer limit = SearchService.getMaxClauseCount();

jainankitk · 2024-05-15T00:06:19Z

server/src/main/java/org/opensearch/search/SearchService.java

+        IndexSearcher.setMaxClauseCount(INDICES_MAX_CLAUSE_COUNT_SETTING.get(settings));
+        clusterService.getClusterSettings().addSettingsUpdateConsumer(INDICES_MAX_CLAUSE_COUNT_SETTING, IndexSearcher::setMaxClauseCount);


It is only possible to override this setting at the cluster level since we'd want all indices to have this effect.

In that case, why not change to below:

this.setMaxClauseCount(INDICES_MAX_CLAUSE_COUNT_SETTING.get(settings)); clusterService.getClusterSettings().addSettingsUpdateConsumer(INDICES_MAX_CLAUSE_COUNT_SETTING, this::setMaxClauseCount);

private void setMaxClauseCount(int maxClauseCount) { this.maxClauseCount = maxClauseCount; IndexSearcher.setMaxClauseCount(maxClauseCount); } // Used in SearchModule above public int getMaxClauseCount() { return this.maxClauseCount; }

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

github-actions · 2024-06-05T17:23:11Z

❌ Gradle check result for b11ca0b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-06-05T17:47:12Z

❌ Gradle check result for a65cecc: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-06-05T18:07:19Z

❌ Gradle check result for de6d547: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-06-05T18:13:15Z

❌ Gradle check result for 05f8d30: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-06-05T18:18:32Z

❌ Gradle check result for 05f8d30: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-06-05T18:37:24Z

❌ Gradle check result for 55496a4: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

github-actions · 2024-06-05T19:35:30Z

❌ Gradle check result for 15d3f96: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

harshavamsi · 2024-06-05T23:39:27Z

@msfroh @reta @dblock could you take a look?

jainankitk

LGTM!

jainankitk · 2024-06-05T23:44:19Z

server/src/main/java/org/opensearch/index/search/QueryParserHelper.java

@@ -177,7 +177,7 @@ static Map<String, Float> resolveMappingField(
    }

    static void checkForTooManyFields(int numberOfFields, QueryShardContext context, @Nullable String inputPattern) {
-        Integer limit = SearchModule.INDICES_MAX_CLAUSE_COUNT_SETTING.get(context.getIndexSettings().getSettings());
+        int limit = SearchService.INDICES_MAX_CLAUSE_COUNT_SETTING.get(context.getIndexSettings().getSettings());


Why not have getMaxClauseCount in SearchService as suggested earlier as well:

public int getMaxClauseCount() { return this.maxClauseCount; }

So, I think the problem there would be to make maxClauseCount static. If we do make it static the clause count is not dynamically update-able. I could be wrong here

No, you're right. Also, the QueryParserHelper class is helper, so makes sense for it to be static and not have the SearchService instance injected. Thanks for clarifying!

github-actions · 2024-06-06T00:12:16Z

❌ Gradle check result for 75331e8: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

harshavamsi · 2024-06-06T00:15:32Z

❌ Gradle check result for 75331e8: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

#13600

reta · 2024-06-06T14:54:55Z

server/src/main/java/org/opensearch/search/SearchService.java

@@ -465,6 +480,11 @@ private void setLowLevelCancellation(Boolean lowLevelCancellation) {
        this.lowLevelCancellation = lowLevelCancellation;
    }

+    private void setMaxClauseCount(int maxClauseCount) {
+        this.maxClauseCount = maxClauseCount;
+        IndexSearcher.setMaxClauseCount(maxClauseCount);


Ok, this is the only shaky place I see: trying to apply volatile semantics (maxClauseCount) to non-volatile static variable (IndexSearcher::maxClauseCount). It is certainly safer to keep this setting static, @msfroh wdyt?

@reta is there any way forward other than making IndexSearcher::maxClauseCount volatile within lucene itself? I understand the semantics of this affect per shard state where some shards will reference the old clause count and new ones will have the new count but eventually consistency will prevail

@reta is there any way forward other than making IndexSearcher::maxClauseCount volatile within lucene itself?

No (AFAIK), we need changes in Apache Lucene (either non static args or thread friendly primitives). We just need to accept the limitation may be and document that behaviour.

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

github-actions · 2024-06-06T19:06:30Z

❌ Gradle check result for a265a2e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-06-06T19:24:35Z

❌ Gradle check result for 7ede1eb: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

github-actions · 2024-06-06T20:22:39Z

✅ Gradle check result for dd840fa: SUCCESS

codecov · 2024-06-06T20:24:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.71%. Comparing base (b15cb0c) to head (dd840fa).
Report is 363 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #13568      +/-   ##
============================================
+ Coverage     71.42%   71.71%   +0.29%     
- Complexity    59978    61529    +1551     
============================================
  Files          4985     5081      +96     
  Lines        282275   289040    +6765     
  Branches      40946    41826     +880     
============================================
+ Hits         201603   207295    +5692     
- Misses        63999    64635     +636     
- Partials      16673    17110     +437

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Set INDICES_MAX_CLAUSE_COUNT dynamically

dd140e2

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

jainankitk reviewed May 6, 2024

View reviewed changes

harshavamsi force-pushed the dynamic_max_clause branch from 6ccaa23 to 28d5525 Compare May 7, 2024 00:24

Adds new test to verify clause count is updated

4ff1d13

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

harshavamsi force-pushed the dynamic_max_clause branch from 28d5525 to 4ff1d13 Compare May 7, 2024 00:27

Changelog

6321ca8

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

harshavamsi marked this pull request as ready for review May 7, 2024 00:58

harshavamsi requested review from anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, dbwiddis, dreamer-89, gbbafna, kotwanikunal, mch2, msfroh, nknize, owaiskazi19, reta, Rishikesh1159, sachinpkale, saratvemulapalli, shwetathareja, sohami, tlfeng and VachaShah as code owners May 7, 2024 00:58

jainankitk reviewed May 15, 2024

View reviewed changes

Merge main

05f8d30

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

harshavamsi force-pushed the dynamic_max_clause branch from 05f8d30 to b11ca0b Compare June 5, 2024 17:16

harshavamsi requested review from jed326 and peternied as code owners June 5, 2024 17:16

harshavamsi force-pushed the dynamic_max_clause branch from b11ca0b to 05f8d30 Compare June 5, 2024 17:17

Merge main

a65cecc

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

Revert static

15d3f96

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

harshavamsi force-pushed the dynamic_max_clause branch from 55496a4 to 15d3f96 Compare June 5, 2024 18:40

Update test case to change defaults

75331e8

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

harshavamsi added the v2.15.0 Issues and PRs related to version 2.15.0 label Jun 5, 2024

jainankitk approved these changes Jun 5, 2024

View reviewed changes

reta reviewed Jun 6, 2024

View reviewed changes

Merge from main

7ede1eb

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

harshavamsi force-pushed the dynamic_max_clause branch from a265a2e to 7ede1eb Compare June 6, 2024 19:04

Spotless

dd840fa

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set INDICES_MAX_CLAUSE_COUNT dynamically #13568

Set INDICES_MAX_CLAUSE_COUNT dynamically #13568

harshavamsi commented May 6, 2024 •

edited

github-actions bot commented May 6, 2024

jainankitk May 6, 2024

harshavamsi May 7, 2024

jainankitk May 10, 2024

harshavamsi May 11, 2024

jainankitk May 15, 2024

harshavamsi Jun 5, 2024

github-actions bot commented May 7, 2024

jainankitk May 15, 2024

jainankitk May 15, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

harshavamsi commented Jun 5, 2024

jainankitk left a comment

jainankitk Jun 5, 2024

harshavamsi Jun 5, 2024

jainankitk Jun 6, 2024

github-actions bot commented Jun 6, 2024

harshavamsi commented Jun 6, 2024

reta Jun 6, 2024

harshavamsi Jun 6, 2024

reta Jun 6, 2024

github-actions bot commented Jun 6, 2024

github-actions bot commented Jun 6, 2024

github-actions bot commented Jun 6, 2024

codecov bot commented Jun 6, 2024

		IndexSearcher.setMaxClauseCount(INDICES_MAX_CLAUSE_COUNT_SETTING.get(settings));
		clusterService.getClusterSettings().addSettingsUpdateConsumer(INDICES_MAX_CLAUSE_COUNT_SETTING, IndexSearcher::setMaxClauseCount);

Set INDICES_MAX_CLAUSE_COUNT dynamically #13568

Are you sure you want to change the base?

Set INDICES_MAX_CLAUSE_COUNT dynamically #13568

Conversation

harshavamsi commented May 6, 2024 • edited

Description

Related Issues

Check List

github-actions bot commented May 6, 2024

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

github-actions bot commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

harshavamsi commented Jun 5, 2024

jainankitk left a 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

github-actions bot commented Jun 6, 2024

harshavamsi commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 6, 2024

github-actions bot commented Jun 6, 2024

github-actions bot commented Jun 6, 2024

codecov bot commented Jun 6, 2024

Codecov Report

harshavamsi commented May 6, 2024 •

edited