Stop processing search requests when _msearch is canceled #17005
Conversation
msfroh
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
jainankitk,
kotwanikunal,
linuxpi,
mch2,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
|
❌ Gradle check result for b88b6d5: 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? |
Prior to this fix, the _msearch API would keep running search requests even after being canceled. With this change, we explicitly check if the task has been canceled before kicking off subsequent requests. Signed-off-by: Michael Froh <[email protected]>
msfroh
force-pushed
the
cancel_msearch
branch
from
b88b6d5 to
30d127a
Compare
|
❕ Gradle check result for 30d127a: 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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17005 +/- ##
============================================
+ Coverage 72.21% 72.23% +0.02%
- Complexity 65289 65309 +20
============================================
Files 5301 5301
Lines 303725 303760 +35
Branches 44008 44015 +7
============================================
+ Hits 219340 219431 +91
+ Misses 66394 66330 -64
- Partials 17991 17999 +8 ☔ View full report in Codecov by Sentry. |
| @@ -193,7 +195,7 @@ private void handleResponse(final int responseSlot, final MultiSearchResponse.It | |||
| if (responseCounter.decrementAndGet() == 0) { | |||
| assert requests.isEmpty(); | |||
| finish(); | |||
| } else { | |||
| } else if (isCancelled(request.request.getParentTask()) == false) { | |||
There was a problem hiding this comment.
I think this makes sense but we need to populate the response slot with the error (task was cancelled or alike), what do you think?
There was a problem hiding this comment.
I could be mistaken, but I think if the parent task gets canceled, then an error response is already sent back to the client.
The customer ticket that prompted this issue involved the client closing the connection (due to timeout). In that case, there is nobody left to receive the response.
I'll try hacking something together with explicit task cancelation to see what the client receives.
There was a problem hiding this comment.
I am not very familiar with this API, but intuitively, we may have a case when some requests are completed (before cancellation) and some would be canceled (after cancellation). So we won't be returning partial results, right? (just cancellation for the whole request)
reta
added
backport 2.x
Description
Prior to this fix, the _msearch API would keep running search requests even after being canceled. With this change, we explicitly check if the task has been canceled before kicking off subsequent requests.
Related Issues
Resolves #17004
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.