From 43884496262f71aa3f33b34ac2f2271959dbf12a Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Tue, 27 Oct 2020 19:43:02 -0400 Subject: [PATCH] Ignore cancellation error when search is cancelled (#64240) Since #63520, we will cancel a search that hits shard failures and does not accept partial results. However, that change can return the wrong HTTP code for bad requests (from 4xx to 5xx) due to the cancellation. Relates #63520 Closes #64012 Closes #63702 --- .../action/search/AbstractSearchAsyncAction.java | 10 ++++++++-- .../xpack/search/AsyncSearchActionIT.java | 1 - 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java b/server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java index 6fa108cd59b79..0653d79f31e84 100644 --- a/server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java @@ -44,6 +44,7 @@ import org.elasticsearch.search.internal.InternalSearchResponse; import org.elasticsearch.search.internal.SearchContext; import org.elasticsearch.search.internal.ShardSearchRequest; +import org.elasticsearch.tasks.TaskCancelledException; import org.elasticsearch.transport.Transport; import java.util.ArrayDeque; @@ -437,8 +438,9 @@ protected void onShardGroupFailure(int shardIndex, SearchShardTarget shardTarget */ @Override public final void onShardFailure(final int shardIndex, @Nullable SearchShardTarget shardTarget, Exception e) { - // we don't aggregate shard failures on non active shards (but do keep the header counts right) - if (TransportActions.isShardNotAvailableException(e) == false) { + // we don't aggregate shard failures on non active shards and failures due to the internal cancellation, + // but do keep the header counts right + if (TransportActions.isShardNotAvailableException(e) == false && (requestCancelled.get() && isTaskCancelledException(e)) == false) { AtomicArray shardFailures = this.shardFailures.get(); // lazily create shard failures, so we can early build the empty shard failure list in most cases (no failures) if (shardFailures == null) { // this is double checked locking but it's fine since SetOnce uses a volatile read internally @@ -469,6 +471,10 @@ public final void onShardFailure(final int shardIndex, @Nullable SearchShardTarg results.consumeShardFailure(shardIndex); } + private static boolean isTaskCancelledException(Exception e) { + return ExceptionsHelper.unwrapCausesAndSuppressed(e, ex -> ex instanceof TaskCancelledException).isPresent(); + } + /** * Executed once for every successful shard level request. * @param result the result returned form the shard diff --git a/x-pack/plugin/async-search/src/internalClusterTest/java/org/elasticsearch/xpack/search/AsyncSearchActionIT.java b/x-pack/plugin/async-search/src/internalClusterTest/java/org/elasticsearch/xpack/search/AsyncSearchActionIT.java index 919177be65fa6..b9e2232e0f61b 100644 --- a/x-pack/plugin/async-search/src/internalClusterTest/java/org/elasticsearch/xpack/search/AsyncSearchActionIT.java +++ b/x-pack/plugin/async-search/src/internalClusterTest/java/org/elasticsearch/xpack/search/AsyncSearchActionIT.java @@ -406,7 +406,6 @@ public void testRemoveAsyncIndex() throws Exception { ensureTaskRemoval(newResp.getId()); } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/63702") public void testSearchPhaseFailureNoCause() throws Exception { SubmitAsyncSearchRequest request = new SubmitAsyncSearchRequest(indexName); request.setKeepOnCompletion(true);