-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add task cancellation check in aggregation code paths #18426
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
Add task cancellation check in aggregation code paths #18426
Conversation
Signed-off-by: Kaushal Kumar <[email protected]>
Signed-off-by: Kaushal Kumar <[email protected]>
Signed-off-by: Kaushal Kumar <[email protected]>
Signed-off-by: Kaushal Kumar <[email protected]>
Signed-off-by: Kaushal Kumar <[email protected]>
Signed-off-by: Kaushal Kumar <[email protected]>
Signed-off-by: Kaushal Kumar <[email protected]>
{"run-benchmark-test": "id_1"} |
Signed-off-by: Kaushal Kumar <[email protected]>
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/3327/ . Final results will be published once the job is completed. |
Signed-off-by: Kaushal Kumar <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18426 +/- ##
============================================
- Coverage 72.74% 72.66% -0.08%
+ Complexity 67767 67686 -81
============================================
Files 5497 5499 +2
Lines 311815 311890 +75
Branches 45261 45273 +12
============================================
- Hits 226822 226630 -192
- Misses 66504 66727 +223
- Partials 18489 18533 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/3327/
|
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/107/
|
{"run-benchmark-test": "id_3"} |
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/3328/ . Final results will be published once the job is completed. |
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/3328/
|
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/108/
|
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 @kaushalmahi12, had a few small comments. For completeness, could you also share an example of what a search request looks like if it gets cancelled in the Aggregation phase? Just want to make sure there's no difference from the user perspective if the search gets cancelled in the query phase or the aggregation phase.
...internalClusterTest/java/org/opensearch/search/aggregations/AggregatorCancellationTests.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/search/DefaultSearchContext.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/search/aggregations/AggregatorBase.java
Outdated
Show resolved
Hide resolved
Thanks @jed326 for reviewing this change
I think you mean request here In current change it will look like following
But if we make the Exception type to task cancellation then it might change slightly Note that this request was fired in a single node single shard index env |
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 for making the changes! Only other thing I can think of is if we want to track the aggs that don't support cancellation today. And how we want to inform plugin developers that they can add cancellation support like so.
❌ Gradle check result for 4e0f814: 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: Kaushal Kumar <[email protected]>
4e0f814
to
f072cdc
Compare
--------- Signed-off-by: Kaushal Kumar <[email protected]> (cherry picked from commit 3078649) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…ject#18426) --------- Signed-off-by: Kaushal Kumar <[email protected]>
…ject#18426) --------- Signed-off-by: Kaushal Kumar <[email protected]>Signed-off-by: TJ Neuenfeldt <[email protected]>
…ject#18426) --------- Signed-off-by: Kaushal Kumar <[email protected]>
Description
This change adds cancellation checks for nested and bucket aggregations. This change will solve the long running cancelled queries stuck in aggregation code paths both at shard and co-ordinator level.
This change takes care of cancellation of request during query phase where the
InternalAggregation
s are built but it is not present in fetch phase where we transform these into final results.We will create a separate PR for introducing cancellation checks in fetch phase as a follow up to this change.
Related Issues
Resolves #15413
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.