Resume driver when failing to fetch pages #106392
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dnhatn
force-pushed
the
exchange-deadlock
branch
from
c33e763
to
63fcb10
Compare
dnhatn
changed the title
dnhatn
commented
Mar 18, 2024
| @@ -203,6 +203,7 @@ void onSinkFailed(Exception originEx) { | |||
| } | |||
| return first; | |||
| }); | |||
| buffer.waitForReading().onResponse(null); // resume the Driver if it is being blocked on reading | |||
There was a problem hiding this comment.
We can notify about the failure, but I think it's simpler just to resume and let the driver handle the error, as if it hadn't been blocked before.
elasticsearchmachine
added
the
Team:Analytics
|
Hi @dnhatn, I've created a changelog YAML for you. |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
nik9000
approved these changes
Mar 18, 2024
| @@ -690,6 +690,7 @@ private void runComputeOnDataNode( | |||
| dataNodeRequestExecutor.start(); | |||
| // run the node-level reduction | |||
| var externalSink = exchangeService.getSinkHandler(externalId); | |||
| task.addListener(() -> exchangeService.finishSinkHandler(externalId, new TaskCancelledException(task.getReasonCancelled()))); | |||
There was a problem hiding this comment.
Could you stick @Nullable on the exception argument to finishSinkHandler and add a note that it'll fire any errors into the sync - if it is still running. Or something like that.
|
Thanks Nik! |
dnhatn
added
the
auto-backport-and-merge
|
Backport to 8.13 in #106436 |
dnhatn
added a commit
to dnhatn/elasticsearch
that referenced
this pull request
Mar 18, 2024
I investigated a heap attack test failure and found that an ESQL request was stuck. This occurred in the following: 1. The ExchangeSource on the coordinator was blocked on reading because there were no available pages. 2. Meanwhile, the ExchangeSink on the data node had pages ready for fetching. 3. When an exchange request tried to fetch pages, it failed due to a CircuitBreakingException. Despite the failure, no cancellation was triggered because the status of the ExchangeSource on the coordinator remained unchanged. To fix this issue, this PR introduces two changes: Resumes the ExchangeSourceOperator and Driver on the coordinator, eventually allowing the coordinator to trigger cancellation of the request when failing to fetch pages. Ensures that an exchange sink on the data nodes fails when a data node request is cancelled. This callback was inadvertently omitted when introducing the node-level reduction in Run empty reduction node level on data nodes elastic#106204. I plan to spend some time to harden the exchange and compute service. Closes elastic#106262
dnhatn
added a commit
that referenced
this pull request
Mar 18, 2024
I investigated a heap attack test failure and found that an ESQL request was stuck. This occurred in the following: 1. The ExchangeSource on the coordinator was blocked on reading because there were no available pages. 2. Meanwhile, the ExchangeSink on the data node had pages ready for fetching. 3. When an exchange request tried to fetch pages, it failed due to a CircuitBreakingException. Despite the failure, no cancellation was triggered because the status of the ExchangeSource on the coordinator remained unchanged. To fix this issue, this PR introduces two changes: Resumes the ExchangeSourceOperator and Driver on the coordinator, eventually allowing the coordinator to trigger cancellation of the request when failing to fetch pages. Ensures that an exchange sink on the data nodes fails when a data node request is cancelled. This callback was inadvertently omitted when introducing the node-level reduction in Run empty reduction node level on data nodes #106204. I plan to spend some time to harden the exchange and compute service. Closes #106262
dnhatn
removed
backport pending
auto-backport-and-merge
This was referenced Mar 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I investigated a heap attack test failure and found that an ESQL request was stuck. This occurred in the following:
To fix this issue, this PR introduces two changes:
Resumes the ExchangeSourceOperator and Driver on the coordinator, eventually allowing the coordinator to trigger cancellation of the request when failing to fetch pages.
Ensures that an exchange sink on the data nodes fails when a data node request is cancelled. This callback was inadvertently omitted when introducing the node-level reduction in Run empty reduction node level on data nodes #106204.
I plan to spend some time to harden the exchange and compute service.
Closes #106262