add cancellation check in FetchPhase for Agg reductions

[kaushalmahi12]

Description

This change introduces cancellation checks for early termination in FetchPhase during shard level reduce and co-ordinator level reduce.

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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.

[kkhatua]

Follow up PR to #18426 and covering the FetchPhase

Tagging @jainankitk , @jed326 and @sgup432 for review

[github-actions]

❌ Gradle check result for 4093508: 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?

[kaushalmahi12]

Looking into test failures

[github-actions]

✅ Gradle check result for e610d4a: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 93.87755% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.74%. Comparing base (223f9fd) to head (e610d4a).
⚠️ Report is 817 commits behind head on main.

Files with missing lines	Patch %	Lines
...pensearch/action/search/TransportSearchAction.java	50.00%	2 Missing ⚠️
...pensearch/action/search/SearchPhaseController.java	90.90%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #18673      +/-   ##
============================================
+ Coverage     72.72%   72.74%   +0.02%     
- Complexity    68409    68439      +30     
============================================
  Files          5566     5568       +2     
  Lines        314292   314369      +77     
  Branches      45579    45586       +7     
============================================
+ Hits         228554   228691     +137     
+ Misses        67153    67105      -48     
+ Partials      18585    18573      -12     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sgup432]

I see we are changing all the desired method signatures to pass the cancellation checks. Doesn't look a nice way to do it.

Instead, I was thinking whether we can create a generic framework(or maybe reuse existing) where we can pass this isTaskCancelled in a more efficient way without change method signatures?
Like maybe storing this info in ThreadLocal(or reusing ThreadContext) where every thread has this info and which can also be propagated along? And we can directly call a static method and fetch that info.

This way we could use this framework for later usecases as well to extend the soft cancellation checks.

[kaushalmahi12]

Thanks @sgup432 ! It is a valid suggestion and I agree that the propagation of task cancellation is not very clean but even to pass the ThreadContext via ThreadPool still requires propagating threadPool. In addition to this threadContext is exposed to the whole lifecycle of search request it becomes very hard to maintain the validity of the state since any new change can stash or restore it.

I feel maybe we need to give it more thought to refactor the query lifecycle in such a way that it is less intrusive to access the request state.

[opensearch-trigger-bot]

This PR is stalled because it has been open for 30 days with no activity.

[jainankitk]

Thanks @kaushalmahi12 for raising this PR. While the functionality itself of being able to effectively cancel the long running search tasks is really useful, we should look into better ways of achieving the same to also ensure the backward compatibility across versions.

[jainankitk]

@kaushalmahi12 - Can we move this PR into draft state until we resume active work on it?

[opensearch-trigger-bot]

This PR is stalled because it has been open for 30 days with no activity.

[opensearch-trigger-bot]

This PR is stalled because it has been open for 30 days with no activity.