Handle all exceptions in data nodes can match #117469

javanna · 2024-11-25T12:48:41Z

During the can match phase, prior to the query phase, we may have exceptions that are returned back to the coordinating node, handled gracefully as if the shard returned canMatch=true.

During the query phase, we perform an additional rewrite and can match phase to eventually shortcut the query phase for the shard. That needs to handle exceptions as well. Currently, an exception there causes shard failures, while we should rather go ahead and execute the query on the shard.

The issue is addressed by folding the exception handling in SearchService#canMatch, rather than leaving it to its consumers. There isn't a single usecase where a failure on can match does not translate to canMatch=true (besides the bug that this fix is addressing). This has the nice benefit that data nodes will stop sending back exceptions for the coordinator to handle, which is unnecessary given that the handling is always the same.

Closes #104994

During the can match phase, prior to the query phase, we may have exceptions that are returned back to the coordinating node, handled gracefully as if the shard returned canMatch=true. During the query phase, we perform an additional rewrite and can match phase to eventually shortcut the query phase for the shard. That needs to handle exceptions as well. Currently, an exception there causes shard failures, while we should rather go ahead and execute the query on the shard. Closes elastic#104994

elasticsearchmachine · 2024-11-25T12:49:06Z

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

javanna · 2024-11-25T12:50:49Z

server/src/main/java/org/elasticsearch/search/SearchService.java

+        }
+    }
+
+    static class CanMatchContext implements Releasable {


most of the changes in SearchService have a simple goal: make the canMatch method unit testable. Given how difficult it is to recreate a SearchService instance (no wonder it requires spinning up a node like we do in SearchServiceTests), I went for making the method static and have some extensible way to provide what it needs as an argument.

javanna · 2024-11-25T12:52:15Z

server/src/main/java/org/elasticsearch/search/SearchService.java

+                        final MinAndMax<?> minMax = sortBuilder != null ? FieldSortBuilder.getMinMaxOrNull(context, sortBuilder) : null;
+                        return new CanMatchShardResponse(true, minMax);
+                    } catch (Exception e) {
+                        return new CanMatchShardResponse(true, null);


This is the core of the change: I am thinking that we should perhaps move the catching entirely to this method. Why send back exceptions when they are consumed by the coord node as can match = true, min max = null? The lack of exception handling in this method has caused this bug in the first place, because it requires all its callers to handle exceptions.

I have now replaced the catch with a broader one and removed the specific one that I had initially added.

javanna · 2024-11-25T12:53:18Z

server/src/main/java/org/elasticsearch/search/SearchService.java

-                    l.onFailure(exc);
-                    return;
+                    // if can_match throws for some reason, we go ahead with the query phase
+                    canMatchResp = new CanMatchShardResponse(true, null);


I added a targeted catch exception for sorting related exceptions, but we still need here a broader one. Based on my proposal below to handle all exceptions in the can match method, perhaps this can go away.

I removed this, given that exception handling is now in callers code.

elasticsearchmachine · 2024-12-02T20:12:56Z

Pinging @elastic/es-search-foundations (Team:Search Foundations)

javanna · 2024-12-02T20:14:06Z

server/src/main/java/org/elasticsearch/search/SearchService.java

@@ -1619,6 +1634,7 @@ public void canMatch(CanMatchNodeRequest request, ActionListener<CanMatchNodeRes
        final List<CanMatchNodeResponse.ResponseOrFailure> responses = new ArrayList<>(shardLevelRequests.size());
        for (var shardLevelRequest : shardLevelRequests) {
            try {
+                // TODO remove the exception handling as it's now in canMatch itself
                responses.add(new CanMatchNodeResponse.ResponseOrFailure(canMatch(request.createShardSearchRequest(shardLevelRequest))));


I left this for a follow-up as I need to add a new transport version and bw comp handling.

javanna · 2024-12-02T20:15:58Z

server/src/test/java/org/elasticsearch/search/SearchServiceSingleNodeTests.java

+ */
+package org.elasticsearch.search;
+
+import org.apache.lucene.index.DirectoryReader;


The diff is hard to follow, but I just renamed the existing SearchServiceTests to SearchServiceSingleNodeTests, and added a new test class with unit tests only, which I called SearchServiceTests

Thanks for the review aid here

javanna · 2024-12-04T13:33:49Z

...ces/src/internalClusterTest/java/org/elasticsearch/index/engine/frozen/FrozenIndexTests.java

+                        null
+                    )
+                ).canMatch()
+            );


the previous version of the assertion came from retrying PIT requests on search context missing. I don't think it's a good reason to make can match fail on it though, hence I adjusted the test. Retries will be attempted in the query phase.

andreidan

Thanks for fixing this Luca.

Left a couple of minor, optional, suggestions.

server/src/main/java/org/elasticsearch/search/SearchService.java

andreidan · 2024-12-09T09:42:23Z

server/src/test/java/org/elasticsearch/search/SearchServiceSingleNodeTests.java

+ */
+package org.elasticsearch.search;
+
+import org.apache.lucene.index.DirectoryReader;


Thanks for the review aid here

original-brownbear

LGTM, would have been nice to do this without the new context class but I see the point :) but 🤞 I guess this goes away somewhat shortly anyway :)

elasticsearchmachine · 2024-12-10T21:55:27Z

💔 Backport failed

Status	Branch	Result
❌	8.16	Commit could not be cherrypicked due to conflicts
❌	8.17	Commit could not be cherrypicked due to conflicts
❌	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 117469

During the can match phase, prior to the query phase, we may have exceptions that are returned back to the coordinating node, handled gracefully as if the shard returned canMatch=true. During the query phase, we perform an additional rewrite and can match phase to eventually shortcut the query phase for the shard. That needs to handle exceptions as well. Currently, an exception there causes shard failures, while we should rather go ahead and execute the query on the shard. Instead of adding another try catch on consumers code, this commit adds exception handling to the method itself so that it can no longer throw exceptions and similar mistakes can no longer be made in the future. At the same time, this commit makes the can match method more easily testable without requiring a full-blown SearchService instance. Closes elastic#104994

* Handle all exceptions in data nodes can match (#117469) During the can match phase, prior to the query phase, we may have exceptions that are returned back to the coordinating node, handled gracefully as if the shard returned canMatch=true. During the query phase, we perform an additional rewrite and can match phase to eventually shortcut the query phase for the shard. That needs to handle exceptions as well. Currently, an exception there causes shard failures, while we should rather go ahead and execute the query on the shard. Instead of adding another try catch on consumers code, this commit adds exception handling to the method itself so that it can no longer throw exceptions and similar mistakes can no longer be made in the future. At the same time, this commit makes the can match method more easily testable without requiring a full-blown SearchService instance. Closes #104994 * fix compile

…lastic#118533) * Handle all exceptions in data nodes can match (elastic#117469) During the can match phase, prior to the query phase, we may have exceptions that are returned back to the coordinating node, handled gracefully as if the shard returned canMatch=true. During the query phase, we perform an additional rewrite and can match phase to eventually shortcut the query phase for the shard. That needs to handle exceptions as well. Currently, an exception there causes shard failures, while we should rather go ahead and execute the query on the shard. Instead of adding another try catch on consumers code, this commit adds exception handling to the method itself so that it can no longer throw exceptions and similar mistakes can no longer be made in the future. At the same time, this commit makes the can match method more easily testable without requiring a full-blown SearchService instance. Closes elastic#104994 * fix compile

… (#118570) * Handle all exceptions in data nodes can match (#117469) During the can match phase, prior to the query phase, we may have exceptions that are returned back to the coordinating node, handled gracefully as if the shard returned canMatch=true. During the query phase, we perform an additional rewrite and can match phase to eventually shortcut the query phase for the shard. That needs to handle exceptions as well. Currently, an exception there causes shard failures, while we should rather go ahead and execute the query on the shard. Instead of adding another try catch on consumers code, this commit adds exception handling to the method itself so that it can no longer throw exceptions and similar mistakes can no longer be made in the future. At the same time, this commit makes the can match method more easily testable without requiring a full-blown SearchService instance. Closes #104994 * fix compile

… (#118572) * Handle all exceptions in data nodes can match (#117469) During the can match phase, prior to the query phase, we may have exceptions that are returned back to the coordinating node, handled gracefully as if the shard returned canMatch=true. During the query phase, we perform an additional rewrite and can match phase to eventually shortcut the query phase for the shard. That needs to handle exceptions as well. Currently, an exception there causes shard failures, while we should rather go ahead and execute the query on the shard. Instead of adding another try catch on consumers code, this commit adds exception handling to the method itself so that it can no longer throw exceptions and similar mistakes can no longer be made in the future. At the same time, this commit makes the can match method more easily testable without requiring a full-blown SearchService instance. Closes #104994 * fix compile

javanna added >bug :Search Foundations/Search Catch all for Search Foundations v9.0.0 v8.16.2 v8.17.1 labels Nov 25, 2024

Update docs/changelog/117469.yaml

cbbe41f

javanna commented Nov 25, 2024

View reviewed changes

javanna requested a review from original-brownbear November 25, 2024 12:53

javanna mentioned this pull request Nov 25, 2024

Unsupported Operation Exception querying Frozen Tier data #104994

Closed

javanna added 3 commits December 2, 2024 19:54

Merge branch 'main' into fix/can_match_query_handle_exception

31151aa

iter

07cbf71

iter

fa097a1

javanna marked this pull request as ready for review December 2, 2024 20:12

elasticsearchmachine added the Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch label Dec 2, 2024

javanna commented Dec 2, 2024

View reviewed changes

iter

95a38e7

javanna changed the title ~~Handle exceptions in query phase can match~~ Handle all exceptions in data nodes can match Dec 2, 2024

javanna and others added 5 commits December 2, 2024 21:21

Merge branch 'main' into fix/can_match_query_handle_exception

619c2b2

iter

9552291

spotless

60ee1c6

Merge branch 'main' into fix/can_match_query_handle_exception

e8061b7

Merge branch 'main' into fix/can_match_query_handle_exception

7c52376

javanna commented Dec 4, 2024

View reviewed changes

javanna removed the v8.16.2 label Dec 4, 2024

javanna added the v8.18.0 label Dec 4, 2024

javanna requested a review from andreidan December 4, 2024 16:15

andreidan approved these changes Dec 9, 2024

View reviewed changes

original-brownbear approved these changes Dec 9, 2024

View reviewed changes

javanna added the v8.16.2 label Dec 9, 2024

javanna added 5 commits December 9, 2024 21:15

iter

6462904

Merge branch 'main' into fix/can_match_query_handle_exception

5979d14

iter

1d6bf5f

iter

5c6e0eb

Merge branch 'main' into fix/can_match_query_handle_exception

0009b92

javanna added the auto-backport Automatically create backport pull requests when merged label Dec 10, 2024

javanna merged commit 730f42a into elastic:main Dec 10, 2024
16 checks passed

javanna deleted the fix/can_match_query_handle_exception branch December 10, 2024 21:54

elasticsearchmachine added the backport pending label Dec 10, 2024

javanna mentioned this pull request Dec 12, 2024

[8.x] Handle all exceptions in data nodes can match (#117469) #118533

Merged

This was referenced Dec 12, 2024

[8.16] Handle all exceptions in data nodes can match (#117469) #118570

Merged

[8.17] Handle all exceptions in data nodes can match (#117469) (#118533) #118572

Merged

javanna removed the backport pending label Dec 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle all exceptions in data nodes can match #117469

Handle all exceptions in data nodes can match #117469

javanna commented Nov 25, 2024 •

edited

Loading

elasticsearchmachine commented Nov 25, 2024

javanna Nov 25, 2024

javanna Nov 25, 2024

javanna Dec 2, 2024

javanna Nov 25, 2024

javanna Dec 2, 2024

elasticsearchmachine commented Dec 2, 2024

javanna Dec 2, 2024

javanna Dec 2, 2024

andreidan Dec 9, 2024

javanna Dec 4, 2024

andreidan left a comment

andreidan Dec 9, 2024

original-brownbear left a comment

elasticsearchmachine commented Dec 10, 2024

Handle all exceptions in data nodes can match #117469

Handle all exceptions in data nodes can match #117469

Conversation

javanna commented Nov 25, 2024 • edited Loading

elasticsearchmachine commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreidan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Dec 10, 2024

💔 Backport failed

javanna commented Nov 25, 2024 •

edited

Loading