Implement runtime skip_unavailable=true #121240

smalyshev · 2025-01-29T22:17:23Z

When skip_unavailable=true, if remote returns disconnect error or cancellation error at runtime, the cluster is marked as "partial", but the query overall does not fail. If the error happens before the exchange is established, the cluster is marked as "skipped".

elasticsearchmachine · 2025-01-30T19:11:43Z

Hi @smalyshev, I've created a changelog YAML for you.

elasticsearchmachine · 2025-01-30T23:08:31Z

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

quux00

First pass review. Several questions left and a few suggestions for changes, some optional. Once I understand the answers to the questions I left I'll do another round of review.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ComputeService.java

.../plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/TransportEsqlQueryAction.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSessionCCSUtils.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/RemoteListenerGroup.java

quux00 · 2025-01-31T19:29:27Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/RemoteListenerGroup.java

+ }
+ };
+ // Cancel the group on sink failure
+ exchangeRequestListener = createCancellingListener("exchange sink failure", computeListener.acquireAvoid(), finishGroup);


Asking to learn - why does the exchangeRequestListener call acquireAvoid while the clusterRequestListener calls acquireCompute?

acquireCompute collects profiles, which as I understand will be coming through clusterRequestListener reqs. At least as far as I understand that code, it's a bit hairy so I may be wrong.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/RemoteListenerGroup.java

quux00 · 2025-01-31T19:36:07Z

...l/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClustersQueryIT.java

 }

+ // Non-disconnect remote failures still fail the request
+ public void testRemoteFailureSkipUnavailable() throws IOException {


nit: change name to include "True", testRemoteFailureWithSkipUnavailableTrue. Otherwise you have to read a bit of code to figure out the setting.

...l/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClustersQueryIT.java

quux00 · 2025-01-31T19:44:43Z

...nternalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClustersCancellationIT.java

+ assertThat(executionInfo.isPartial(), equalTo(true));
+ EsqlExecutionInfo.Cluster cluster = executionInfo.getCluster(REMOTE_CLUSTER);
+
+ assertThat(cluster.getStatus(), equalTo(EsqlExecutionInfo.Cluster.Status.PARTIAL));


Hmm, I'm confused. I thought cancelling an ES|QL would result in a top level failure, like a 500 status. Is that not right? It returns partial data?

Canceling the remote shouldn't - at least according to our slack conversation. Is it not correct?

dnhatn

I left some comments. Thanks @smalyshev.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/RemoteListenerGroup.java

dnhatn · 2025-01-31T19:40:48Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/RemoteListenerGroup.java

+ ComputeListener computeListener,
+ String clusterAlias,
+ EsqlExecutionInfo executionInfo,
+ ActionListener<Void> delegate


Do we need to pass both ComputeListener and this extra listener?

Looking at the code, this listener is used to complete the ComputeListener is the group is finished, but also is used in the exception clause of ExchangeService.openExchange - so I am not sure how to get rid of it.

dnhatn · 2025-01-31T19:50:50Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/RemoteListenerGroup.java

+ }
+ };
+ // Cancel the group on sink failure
+ exchangeRequestListener = createCancellingListener("exchange sink failure", computeListener.acquireAvoid(), finishGroup);


With this change, if a compute from a remote cluster fails and skip_unavailable=false, or the failure is not remote_unavailable_exceptions, we will not start canceling computes on other clusters, including the local cluster, until the cancellation of the failing cluster complete - which may take time. Can we maintain this behavior? But it's perfectly fine not to maintain it if it's complex.

I'm not sure how to do this - do you have any suggestions?

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ClusterComputeHandler.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSessionCCSUtils.java

...nternalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClustersCancellationIT.java

quux00

Next round review. Can approve on next round after another fix or two. Looks good.

quux00 · 2025-02-03T17:19:03Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlCCSUtils.java

+ ) {
+ return delegate.delegateResponse((l, e) -> {
+ if (EsqlCCSUtils.shouldIgnoreRuntimeError(executionInfo, clusterAlias, e)) {
+ EsqlCCSUtils.markClusterWithFinalStateAndNoShards(executionInfo, clusterAlias, status, e);


nit: you don't need the EsqlCCSUtils. qualifier on the two method calls above.

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/session/EsqlCCSUtilsTests.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ClusterComputeHandler.java

quux00

LGTM - nice work!

elasticsearchmachine · 2025-02-03T22:49:12Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 121240

smalyshev · 2025-02-03T22:56:40Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation

* Implement runtime skip_unavailable=true (#121240) * Implement runtime skip_unavailable=true (cherry picked from commit 2fbec77) # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

* Implement runtime skip_unavailable=true

createLocalIndex() was added in elastic#121240 but is unused now after testCloseSkipUnavailable() was removed in elastic#123054.

createLocalIndex() was added in #121240 but is unused now after testCloseSkipUnavailable() was removed in #123054.

createLocalIndex() was added in elastic#121240 but is unused now after testCloseSkipUnavailable() was removed in elastic#123054.

elasticsearchmachine added v9.0.0 v9.1.0 and removed v9.0.0 labels Jan 29, 2025

smalyshev added :Analytics/ES|QL AKA ESQL v8.19.0 >enhancement labels Jan 30, 2025

smalyshev and others added 6 commits January 30, 2025 16:07

Implement runtime skip_unavailable=true

a8662d8

Better exception check

49ed90e

Refine monitored exceptions

0848dd1

[CI] Auto commit changes from spotless

1629251

Update docs/changelog/121240.yaml

dcd0d78

Add tests

d3c8ef5

smalyshev force-pushed the skip-un-execution branch from 27d76b6 to d3c8ef5 Compare January 30, 2025 23:07

smalyshev requested review from dnhatn and quux00 January 30, 2025 23:08

smalyshev marked this pull request as ready for review January 30, 2025 23:08

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 30, 2025

smalyshev mentioned this pull request Jan 30, 2025

Ignore remote ES|QL execution failures when skip_unavailable=true #116365

Closed

4 tasks

smalyshev and others added 2 commits January 31, 2025 10:46

Make partial to be set when there's skipped or partial

5833317

Merge branch 'main' into skip-un-execution

a33df84

quux00 reviewed Jan 31, 2025

View reviewed changes

Add skip_unavailable=false test

31e0eb4

dnhatn reviewed Jan 31, 2025

View reviewed changes

smalyshev and others added 5 commits January 31, 2025 13:09

Pull feedback

d3b27a1

Make cancel to fail and other feedback

99cc226

Merge branch 'main' into skip-un-execution

1f397e0

[CI] Auto commit changes from spotless

027b156

Refactor the listener code

8ef56eb

smalyshev and others added 2 commits February 3, 2025 08:37

Merge branch 'main' into skip-un-execution

fc98a0e

Fix task for child request

23bdac0

quux00 reviewed Feb 3, 2025

View reviewed changes

smalyshev added 2 commits February 3, 2025 13:23

Add more tests

3eb5b68

Bump timeout to give slow clusters more time

ff431f9

smalyshev force-pushed the skip-un-execution branch from 3b3ae1f to ff431f9 Compare February 3, 2025 20:26

quux00 approved these changes Feb 3, 2025

View reviewed changes

smalyshev added 2 commits February 3, 2025 13:43

Merge branch 'main' into skip-un-execution

c458f9e

Test fixes

c9be7dc

smalyshev added the auto-backport Automatically create backport pull requests when merged label Feb 3, 2025

Fix test

39216e2

smalyshev merged commit 2fbec77 into elastic:main Feb 3, 2025
17 checks passed

elasticsearchmachine added the backport pending label Feb 3, 2025

smalyshev mentioned this pull request Feb 3, 2025

[8.x] Implement runtime skip_unavailable=true (#121240) #121602

Merged

smalyshev removed the backport pending label Feb 4, 2025

fzowl pushed a commit to voyage-ai/elasticsearch that referenced this pull request Feb 4, 2025

Implement runtime skip_unavailable=true (elastic#121240)

8dafdfe

* Implement runtime skip_unavailable=true

JeremyDahlgren mentioned this pull request Aug 14, 2025

Remove CrossClusterCancellationIT.createLocalIndex() #132952

Merged

JeremyDahlgren added a commit that referenced this pull request Aug 14, 2025

Remove CrossClusterCancellationIT.createLocalIndex() (#132952)

3132d61

createLocalIndex() was added in #121240 but is unused now after testCloseSkipUnavailable() was removed in #123054.

Implement runtime skip_unavailable=true #121240

Implement runtime skip_unavailable=true #121240

Uh oh!

Conversation

smalyshev commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Jan 30, 2025

elasticsearchmachine commented Jan 30, 2025

quux00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

smalyshev Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

quux00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

quux00 left a comment

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Feb 3, 2025

💔 Backport failed

smalyshev commented Feb 3, 2025

💚 All backports created successfully

Questions ?

Labels

4 participants

smalyshev commented Jan 29, 2025 •

edited

Loading

smalyshev Jan 31, 2025 •

edited

Loading