Skip to content
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

Fix DataNodeRequestSender #121999

Merged
merged 1 commit into from
Feb 11, 2025
Merged

Fix DataNodeRequestSender #121999

merged 1 commit into from
Feb 11, 2025

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Feb 7, 2025

There are two issues in the current implementation:

  1. We should use the list of shardIds from the request, rather than all targets, when removing failures for shards that have been successfully executed.

  2. We should remove shardIds from the pending list once a failure is reported and abort execution at that point, as the results will be discarded.

Closes #121966

@dnhatn dnhatn marked this pull request as ready for review February 8, 2025 01:48
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Feb 8, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@dnhatn dnhatn enabled auto-merge (squash) February 11, 2025 00:43
@dnhatn dnhatn disabled auto-merge February 11, 2025 00:43
@dnhatn dnhatn merged commit bda99c9 into elastic:main Feb 11, 2025
16 of 17 checks passed
@dnhatn dnhatn deleted the fix-sender branch February 11, 2025 00:44
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Feb 11, 2025
There are two issues in the current implementation:

1. We should use the list of shardIds from the request, rather than all
targets, when removing failures for shards that have been successfully
executed.

2. We should remove shardIds from the pending list once a failure is reported
and abort execution at that point, as the results will be discarded.

Closes elastic#121966
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Feb 11, 2025
There are two issues in the current implementation:

1. We should use the list of shardIds from the request, rather than all
targets, when removing failures for shards that have been successfully
executed.

2. We should remove shardIds from the pending list once a failure is reported
and abort execution at that point, as the results will be discarded.

Closes elastic#121966
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Feb 15, 2025
There are two issues in the current implementation:

1. We should use the list of shardIds from the request, rather than all
targets, when removing failures for shards that have been successfully
executed.

2. We should remove shardIds from the pending list once a failure is reported
and abort execution at that point, as the results will be discarded.

Closes elastic#121966
dnhatn added a commit that referenced this pull request Feb 15, 2025
* Retry ES|QL node requests on shard level failures (#120774)

Today, ES|QL fails fast on any failure. This PR introduces support for
retrying within a cluster when data-node requests fail.

There are two types of failures that occur with data-node requests:
entire request failures and individual shard failures. For individual
shard failures, we can retry the next copies of the failing shards. For
entire request failures, we can retry every shard in the node request if
no pages have been received.

On the handling side, ES|QL executes against a batch of shards
concurrently. Here, we need to track whether any pages have been
produced. If pages have been produced, the entire request must fail.
Otherwise, we can track the failed shards and send them back to the
sender for retries.

There are two decisions around how quickly we should retry:

1. Should we notify the sender of failing shards immediately (via a
different channel) to enable quick retries, or should we accumulate
failures and return them in the final response?

2. What is the maximum number of inflight requests we should allow on
the sending side?

This PR considers failures often occurring when the cluster is under
load or during a rolling upgrade. To prevent retries from adding more
load and to allow the cluster to stabilize, this PR chooses to send
shard failures in the final response and limits the number of inflight
requests to one per data node

Includes #121999

Closes #121966
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.19.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] DataNodeRequestSenderTests testDoNotRetryOnRequestLevelFailure failing
2 participants