No special handling rules for skip_unavailable=true clusters at plan time other than disconnected exceptions by quux00 · Pull Request #120236 · elastic/elasticsearch

quux00 · 2025-01-15T20:34:52Z

For ES|QL, we are moving to limit the scope of the skip_unavailable setting for remote clusters.
Going forward, skip_unavailable will be considered for two scenarios:

inability to connect to a remote cluster ("unavailable")
whether to fail on execution time errors or not (inline with the upcoming allow_partial_search_results work for ES|QL)

This PR reverses the special plan-time handling for skip_unavailable=true clusters that was added in #116348. Remote clusters, regardless of their skip_unavailable setting, will now use the same logic as the local cluster for index expression analysis at plan time, namely:

If any concrete index specified is missing from the cluster, a VerificationException will be thrown
If no matching index/alias/datastream was found on any cluster (even if all were specified with a wildcard), a VerificationException will be thrown

Thus, we no longer require at least one matching index expression for skip_unavailable=false clusters either, as was done in the previous PR referenced above.

…time other than disconnected exceptions

… at plan time

elasticsearchmachine · 2025-01-16T20:22:35Z

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

astefan · 2025-01-20T10:40:21Z

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

+        {
+            String q = "FROM nomatch*,cluster-a:nomatch*";
+            VerificationException e = expectThrows(VerificationException.class, () -> runQuery(q, requestIncludeMeta));
+            assertThat(e.getDetailedMessage(), containsString("Unknown index [cluster-a:nomatch*,nomatch*]"));


Interesting note here: why is the order not kept? User specified nomatch*,cluster-a:nomatch*.

This ordering seems to happen in RemoteClusterAware#groupClusterIndices().

astefan · 2025-01-20T10:45:16Z

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

-                e = expectThrows(VerificationException.class, () -> runQuery(limit0, requestIncludeMeta));
-                assertThat(e.getDetailedMessage(), containsString("Unknown index [cluster-a:nomatch,nomatch*]"));
-            }
+        // an error is thrown if there is a concrete index that does not match


I think you could simplify a bit the entire series of VerificationExceptions here and extract a method with common functionality. For example:

private void expectVerificationExceptionForQuery(String q, String error, Boolean requestIncludeMeta) { VerificationException e = expectThrows(VerificationException.class, () -> runQuery(q, requestIncludeMeta)); assertThat(e.getDetailedMessage(), containsString(error)); String limit0 = q + " | LIMIT 0"; e = expectThrows(VerificationException.class, () -> runQuery(limit0, requestIncludeMeta)); assertThat(e.getDetailedMessage(), containsString(error)); }

IntelliJ tells me I could use the method above in 12 places in this test method.

Good idea. Changed in latest push.

astefan · 2025-01-20T11:06:27Z

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

        String remote2Index = (String) testClusterInfo.get("remote2.index");

        createIndexAliases(numClusters);
-        setSkipUnavailable(REMOTE_CLUSTER_1, true);


My understanding (from the documentation) is that skip_unavailable is false by default, if not explicitly set.

Likely I am missing something, but shouldn't skip_unvailable value not matter in this test method? Meaning, it can either be un-set, set to false or set to true, the VerificationException should be thrown no matter what, when an inexistent index is specified (concrete or as part of a wildcarded expression when 0 indices are found).

My understanding (from the documentation) is that skip_unavailable is false by default, if not explicitly set

That used to be the case but was changed to default to true in 8.15: #105792

shouldn't skip_unvailable value not matter in this test method? Meaning, it can either be un-set, set to false or set to true

That is correct, which is why I removed the explicit setting of skip_unavailable here (since it used to matter). Now it gets set to a random setting by this part of the test setup. It is randomized a test case set up for all tests unless a specific test needs to specifically override it and set a specific value which is what this test used to do. Now it just allows the random value.

Hm... I was slightly mislead by our docs on this, grabbing the first piece of info on the default value I found. We might have a small inconsistency here, even though in other places it is specifically mentioned that the default changed from false to true.

I opened a small PR for this #120592

…lable-plan-time-removal

astefan

LGTM. Thanks

elasticsearchmachine · 2025-01-22T13:41:55Z

💚 Backport successful

Status	Branch	Result
✅	8.x

…time other than disconnected exceptions (elastic#120236) For ES|QL, we are moving to limit the scope of the skip_unavailable setting for remote clusters. Going forward, skip_unavailable will be considered for two scenarios: 1) inability to connect to a remote cluster ("unavailable") 2) whether to fail on execution time errors or not (inline with the upcoming allow_partial_search_results work for ES|QL). This PR reverses the special plan-time handling for skip_unavailable=true clusters that was added in elastic#116348. Remote clusters, regardless of their skip_unavailable setting, will now use the same logic as the local cluster for index expression analysis at plan time, namely: 1) If any concrete index specified is missing from the cluster, a VerificationException will be thrown 2) If no matching index/alias/datastream was found on any cluster (even if all were specified with a wildcard), a VerificationException will be thrown Thus, we no longer require at least one matching index expression for skip_unavailable=false clusters either, as was done in the previous PR referenced above.

…time other than disconnected exceptions (#120236) (#120628) For ES|QL, we are moving to limit the scope of the skip_unavailable setting for remote clusters. Going forward, skip_unavailable will be considered for two scenarios: 1) inability to connect to a remote cluster ("unavailable") 2) whether to fail on execution time errors or not (inline with the upcoming allow_partial_search_results work for ES|QL). This PR reverses the special plan-time handling for skip_unavailable=true clusters that was added in #116348. Remote clusters, regardless of their skip_unavailable setting, will now use the same logic as the local cluster for index expression analysis at plan time, namely: 1) If any concrete index specified is missing from the cluster, a VerificationException will be thrown 2) If no matching index/alias/datastream was found on any cluster (even if all were specified with a wildcard), a VerificationException will be thrown Thus, we no longer require at least one matching index expression for skip_unavailable=false clusters either, as was done in the previous PR referenced above.

quux00 added >non-issue auto-backport Automatically create backport pull requests when merged v9.0.0 v8.18.0 labels Jan 15, 2025

quux00 force-pushed the esql-ccs/skip-unavailable-plan-time-removal branch 2 times, most recently from 9008195 to 93f6d26 Compare January 16, 2025 17:38

quux00 added 4 commits January 16, 2025 15:21

No special handling rules for skip_unavailable=true clusters at plan …

b48c6a4

…time other than disconnected exceptions

Fixed failing tests to align with new model for skip_un=true clusters…

59e2843

… at plan time

Remove System.err println

be26238

Minor cleanup for PR review

d5aea8c

quux00 force-pushed the esql-ccs/skip-unavailable-plan-time-removal branch from 002d89c to d5aea8c Compare January 16, 2025 20:21

quux00 marked this pull request as ready for review January 16, 2025 20:21

quux00 added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL labels Jan 16, 2025

quux00 requested review from astefan, pawankartik-elastic and smalyshev January 16, 2025 20:22

astefan reviewed Jan 20, 2025

View reviewed changes

quux00 added 2 commits January 21, 2025 15:57

Adjusted IT based on PR feedback

3d8599c

Merge remote-tracking branch 'elastic/main' into esql-ccs/skip-unavai…

f63dcd4

…lable-plan-time-removal

quux00 requested a review from astefan January 21, 2025 20:58

astefan approved these changes Jan 22, 2025

View reviewed changes

smalyshev approved these changes Jan 22, 2025

View reviewed changes

pawankartik-elastic approved these changes Jan 22, 2025

View reviewed changes

quux00 merged commit 0de4b92 into elastic:main Jan 22, 2025
16 checks passed

quux00 mentioned this pull request Jan 22, 2025

[8.x] No special handling rules for skip_unavailable=true clusters at plan time other than disconnected exceptions (#120236) #120628

Merged

georgewallace mentioned this pull request May 12, 2025

Update 8.18.0.asciidoc #128028

Merged

smalyshev mentioned this pull request May 19, 2025

ES|QL: Make skip_unavailable catch all errors #128163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No special handling rules for skip_unavailable=true clusters at plan time other than disconnected exceptions#120236

No special handling rules for skip_unavailable=true clusters at plan time other than disconnected exceptions#120236
quux00 merged 6 commits intoelastic:mainfrom
quux00:esql-ccs/skip-unavailable-plan-time-removal

quux00 commented Jan 15, 2025 •

edited

Loading

elasticsearchmachine commented Jan 16, 2025

astefan Jan 20, 2025

pawankartik-elastic Jan 20, 2025

astefan Jan 20, 2025

quux00 Jan 21, 2025

astefan Jan 20, 2025

quux00 Jan 21, 2025 •

edited

Loading

astefan Jan 22, 2025 •

edited

Loading

astefan Jan 22, 2025

astefan left a comment

Uh oh!

elasticsearchmachine commented Jan 22, 2025

Labels

5 participants

Conversation

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

Uh oh!

elasticsearchmachine commented Jan 16, 2025

astefan Jan 20, 2025

Choose a reason for hiding this comment

pawankartik-elastic Jan 20, 2025

Choose a reason for hiding this comment

astefan Jan 20, 2025

Choose a reason for hiding this comment

quux00 Jan 21, 2025

Choose a reason for hiding this comment

astefan Jan 20, 2025

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

astefan Jan 22, 2025

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Jan 22, 2025

💚 Backport successful

Labels

5 participants

quux00 commented Jan 15, 2025 •

edited

Loading

quux00 Jan 21, 2025 •

edited

Loading

astefan Jan 22, 2025 •

edited

Loading