Harden the circuit breaker and failure handle logic in query result consumer

[bowenlan-amzn]

Description

This PR is meant for harden the circuit breaker logic in query result consumer. The main logic change is only at one place, addEstimateAndMaybeBreak(aggsSize) in consumeResult before actually perform any real "consume logic".

On the other hand, considering this is a important but spaghetti code, and possibly need to be improved in the future, some refactoring works are also done.

Circuit Breaker Change

Circuit breaker addEstimateBytesAndMaybeBreak are used twice

1. before consume: query result received at coordinator transport layer, and will be handled by query result consumer. We estimate the heap size of the query result, and check if it breaks the REQUEST circuit breaker.
2. during tryExecuteNext: before doing partial reduce on the buffered query results, we estimate the extra heap size that will be used, and check if it breaks the REQUEST circuit breaker.

Some context about the partial merge logic:

• PendingMerges are the core logic to buffer and reduce shard results in a batched manner without waiting for all results arrived.
• consume buffer the shard result and check if the buffer size reaches threshold, and create merge task from buffer.
• tryExecuteNext poll merge task from queue and perform the partial reduce.

Refactoring around Failure Handling

The rule I followed is to use onMergeFailure to handle any failure we captured. It could be a circuit breaker exception, a task cancellation exception or any exception caught during the partial reduce.

onMergeFailure stores the failure, resets the circuit breaker, and clears the merge task queue. It also sends the cancel search task request to other nodes so they could try stop processing the shard query at their best.

Performance Implication

TermsReduceBenchmark doesn't show any regression

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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.

[github-actions]

❌ Gradle check result for 5802e94: 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]

@kaushalmahi12 - Can you help with review for this code change?

[github-actions]

✅ Gradle check result for 89f9dde: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 83.46457% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.98%. Comparing base (983c4d7) to head (18cbfa0).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...search/action/search/QueryPhaseResultConsumer.java	83.33%	10 Missing and 11 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #19396      +/-   ##
============================================
- Coverage     73.08%   72.98%   -0.11%     
+ Complexity    70491    70382     -109     
============================================
  Files          5712     5712              
  Lines        322762   322754       -8     
  Branches      46743    46744       +1     
============================================
- Hits         235879   235549     -330     
- Misses        67941    68207     +266     
- Partials      18942    18998      +56     

☔ 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.

[github-actions]

❌ Gradle check result for ecc930c: 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]

Thanks @bowenlan-amzn for improving this nightmarish class. Can you also add a diagram how does the existing class works and what are we improving in that. Diagram will definitely help the reviewers to quickly get the context.

[github-actions]

❕ Gradle check result for 92c0bd9: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[github-actions]

❕ Gradle check result for f21f79d: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[github-actions]

❕ Gradle check result for 01aa6e3: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[bowenlan-amzn]

@kaushalmahi12 thanks for the review!
Pushed a commit of renaming "merge" to reduce, and provide better javadoc 18cbfa0 (#19396)

[github-actions]

❌ Gradle check result for 18cbfa0: 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]

✅ Gradle check result for 18cbfa0: SUCCESS