-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Enable dynamic updates to max allowed boolean clauses #1527
Conversation
Can one of the admins verify this patch? |
✅ Gradle Wrapper Validation success 1c75ad30ac28c86a84b5b1673ab93655529a525b |
❌ Gradle Check failure 1c75ad30ac28c86a84b5b1673ab93655529a525b |
❌ Gradle Precommit failure 1c75ad30ac28c86a84b5b1673ab93655529a525b |
✅ Gradle Wrapper Validation success 42b864b0606160829dec8f65609eb043e4532ea1 |
✅ Gradle Precommit success 42b864b0606160829dec8f65609eb043e4532ea1 |
✅ Gradle Check success 42b864b0606160829dec8f65609eb043e4532ea1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1
@@ -302,6 +312,10 @@ public SearchService( | |||
|
|||
lowLevelCancellation = LOW_LEVEL_CANCELLATION_SETTING.get(settings); | |||
clusterService.getClusterSettings().addSettingsUpdateConsumer(LOW_LEVEL_CANCELLATION_SETTING, this::setLowLevelCancellation); | |||
|
|||
BooleanQuery.setMaxClauseCount(INDICES_MAX_CLAUSE_COUNT_SETTING.get(settings)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the concerns regarding the impact of this change. First of all, to acknowledge, making this property adjustable is useful. However implementation-wise, since BooleanQuery::setMaxClauseCount
is static setting, changing it could impact the queries which are already being submitted / executed in a very weird ways. Any thoughts on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the limits get checked at the time of query parse/rewrite - so at shard level, the query will either succeed (below clause limit) or fail (breached limit)
Here are different scenarios:
Initial State: with limit at 1024
- Query with 1500 clauses will be rejected
Transition state: limit raised to 2048 via the dynamic setting added here
- Ongoing/queries submitted concurrent with the settings change (say to raise the threshold) will initially fail with too many clauses (as before)
- Eventual consistency will prevail and all nodes will eventually apply the updated setting(within state lag period)
- In the case of a query spanning multiple shards across nodes, there could be scenario where shards on the node that have applied settings respond successfully and shards on nodes with old setting will return failure and such queries will see partial success during transition (instead of full failure before).
- Eventually post applying of the setting, all queries with clauses < 2048 will get passed
Does this address your concerns? If not, it will help to share the exact scenario you had in mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @malpani , this "... eventual consistency ..." part is essentially what bothers me, not only between nodes but between multiple shards on the same node: it may happen that for the same search request, while it is being distributed between shards, some shards may see old setting whereas others may see new one.
Not sure it is solvable at all, unless we take into account the state of search-related thread pools and delay the change, probably adding documentation notes about the impact of changing this setting on clusters would be sufficient.
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, I will tag you on the doc update PR.
Intra-node consistency probably can be achieved by making it volatile in Lucene. We would still have the inter node eventual consistency so will document this clearly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there another example where we are adjusting a dynamic property in this way or is it the first time? I'm all for YOLOing a setting at runtime, but maybe @nknize has a stronger opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't dug deeper here but thinking out loud, if we could do such check on the coordinator node that spans out the search request, we could probably avoid intra and inter node inconsistencies by rejecting limits breach at the coordinator itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dblock - yes, this is a common approach for updating dynamic settings for example - https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/search/SearchService.java#L294 . the slight uniqueness here is it is updating a static setting.
@setiah - Interesting thought - while this clause validation can be done on coordinator side, I dont want to parse yet again for checking on this failure scenario and affect the happy paths. Background - each shard query does a rewrite based on local stats for that shard (not available on coordinator) - so the per shard rewrite is not avoidable today. One of the long term exploration areas is to do better coordinator side planning and query estimation and this could be revisited then. I want to keep this change simple and dont really see a problem with eventual consistency semantics here (this is inline with other setting updates)
.prepareUpdateSettings() | ||
.setTransientSettings(Settings.builder().put(INDICES_MAX_CLAUSE_COUNT_SETTING.getKey(), 2048)) | ||
); | ||
Thread.sleep(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we check if max_clause_count
value is updated to new value (with smaller sleeps and timeout) before proceeding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done - reduced sleep time from 10 to 1ms (the ideal way is to track via countdown latches but all my local runs pass with even a 1ms sleep so making the simpler change)
Signed-off-by: Ankit Malpani <malpani@amazon.com>
Signed-off-by: Ankit Malpani <malpani@amazon.com>
42b864b
to
bb38d20
Compare
✅ Gradle Wrapper Validation success bb38d20 |
✅ Gradle Wrapper Validation success 65b9934a34eba14bdaad488e5f2730b5b109e05f |
✅ Gradle Precommit success bb38d20 |
✅ Gradle Precommit success 65b9934a34eba14bdaad488e5f2730b5b109e05f |
❌ Gradle Check failure 65b9934a34eba14bdaad488e5f2730b5b109e05f |
i looked into the gradle failure and its coming from an unrelated area - looks like the docker infra for s3 repository tests did not come up cleanly. Here are the relevant logs - could one of the maintainers triage this or let me know what the next steps are?
|
@malpani yeah, seems like broad CI issue, have seen it there #1500 (comment) as well. |
start gradle check |
@malpani amend your commits with |
✅ Gradle Check success 65b9934a34eba14bdaad488e5f2730b5b109e05f |
Signed-off-by: Ankit Malpani <ankit.malpani@gmail.com>
65b9934
to
67fee73
Compare
Signed-off-by: Ankit Malpani <ankit.malpani@gmail.com>
✅ Gradle Wrapper Validation success 67fee73 |
✅ Gradle Wrapper Validation success ed0e6e4 |
✅ Gradle Precommit success 67fee73 |
✅ Gradle Precommit success ed0e6e4 |
fixed this now. The |
the current gradle check failure looks transient as running
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of my concerns were already raised on this PR.
- eventual consistency for in-flight queries
- overall performance degradation
In general I'm concerned about users being too cavalier with this setting. One mis-behaved query may lead to a user blindly increasing this setting instead of fully inspecting and re-working the query to improve performance and suddenly long term search latencies lead to a boiling frog. At least right now the cluster needs to be brought down before blindly changing the parameter.
What version is this targeting?
Lucene 9 refactored BooleanQuery.maxClauseCount
to IndexSearcher.maxClauseCount
and there has been some discussion around further refactoring to be per-searcher instead of global so this can be converted to an IndexScope
setting.
I'm all for the progress not perfection and it is true there are other settings (e.g., search.max_buckets
) that can cause a similar issue but I'm not sure that justifies adding another potential trap?
I think a better approach might be to consider contributing an upstream Lucene 9.1 change to refactor IndexSearcher.maxClauseCount
to be per-index and then change this PR to refactor the current NodeScope
indices.query.bool.max_clause_count
to be IndexScope
index.query.bool.max_clause_count
and release as an enhancement in OpenSearch 2.0. This has the benefit of minimizing the bwc blast radius. In the meantime, users can continue changing this setting w/ a full cluster restart, which I think is a better guardrail than adding this dynamic leniency.
Thanks @nknize for reviewing. Here are my responses
It is an artifact of a distributed cluster state. It is the same with any other dynamic setting on opensearch - am I missing something?
I prefer users get to decide this ie. providing the tradeoff of a slow query as against today's behavior of a failing query with no option to get out of except a full cluster restart or re-working client logic. That is the intent behind making it dynamic.
1.3 - as existing behavior and defaults are unchanged and it is just an extra convenience knob. We have some customers asking for this ability and today it is operationally heavy to update ymls and do rolling restarts as the only way to bump up clause limits
Based on this suggestion, can i assume that you are not concerned about making the setting dynamic :) Index vs cluster level setting is an orthogonal discussion. I am not convinced that index level granularity is needed upfront and hence prefer an incremental approach of roll out with cluster level setting in 1.3 (which uses Lucene 8.x and needs no changes to Lucene). If there is ask from users to get more granular, per index option can be revisited on the next major version of OpenSearch. |
@malpani I think setting |
"...I'm not sure that justifies adding another potential trap?"
this was my point behind "...At least right now the cluster needs to be brought down before blindly changing the parameter." as a guard rail against being overly cavalier about this parameter. This limitation isn't a blocker today, it's just an inconvenience; and one I think is good to prevent against silently harming search performance.
+100 to community decision. IMHO failure in this case is better than a slow query. In the former there's no question as to the cause
If it's index scoped where behavior is impacted on a per index basis as opposed to all queries then yes I'm not opposed to it being dynamic.
Refactoring from a static to a runtime variable on a per IndexSearcher scope is coming at the lucene level regardless of what we decide here. We can influence and expedite that if we'd like and get it done for 9.1. The result is that would have to wait for OpenSearch 2.0 at the earliest (assuming 9.1 is released before OpenSearch 2.0; a safe bet I think) which I think is a net positive to minimize bwc requirements.
Precisely. We'd be shifting from cluster wide I like the idea of contributing upstream to lucene and introducing this as an index level setting in OpenSearch 2.x. |
Thanks, agree that a future migration to index level from cluster level will become an overhead. I will close this PR for now. |
Description
Make indices.query.bool.max_clause_count a dynamic setting
Issues Resolved
#1526
Check List
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.