From bd0a94f250902137febda51c94f3672dc242c3f2 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Fri, 14 Feb 2025 15:28:47 +0100 Subject: [PATCH] Return an empty suggestion when suggest phase times out (#122575) We recently fixed timeout handling in the suggest phase. A test failure on SearchTimeoutIT surfaced an issue with the current approach. In case partial results are allowed, it may happen that some shards time out while executing the suggest phase and some don't. SearchPhaseController assumes that if one shard has suggest results, all of the other shards will have suggest results too. We could address that assertion and check is the search timed out, instead this commit changes timeout handling in the suggest phase to return an empty suggestion instead of null. This seems appropriate in terms of providing some results and makes the assertion about non null suggestions in SearchPhaseController happy. Relates to #122357 Closes #122548 --- docs/changelog/122575.yaml | 6 +++++ muted-tests.yml | 3 --- .../elasticsearch/search/SearchTimeoutIT.java | 3 +-- .../search/query/QueryPhase.java | 26 ++++--------------- .../search/rescore/RescorePhase.java | 9 +++++++ .../search/suggest/SuggestPhase.java | 19 +++++++++----- .../search/query/QueryPhaseTimeoutTests.java | 6 +++-- 7 files changed, 38 insertions(+), 34 deletions(-) create mode 100644 docs/changelog/122575.yaml diff --git a/docs/changelog/122575.yaml b/docs/changelog/122575.yaml new file mode 100644 index 0000000000000..af72c81b9da8c --- /dev/null +++ b/docs/changelog/122575.yaml @@ -0,0 +1,6 @@ +pr: 122575 +summary: Return an empty suggestion when suggest phase times out +area: Suggesters +type: bug +issues: + - 122548 diff --git a/muted-tests.yml b/muted-tests.yml index 4cd59dfd1a52f..b1927e224eb38 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -326,9 +326,6 @@ tests: - class: org.elasticsearch.compute.operator.exchange.ExchangeServiceTests method: testExchangeSourceContinueOnFailure issue: https://github.com/elastic/elasticsearch/issues/122408 -- class: org.elasticsearch.search.SearchTimeoutIT - method: testSuggestTimeoutWithPartialResults - issue: https://github.com/elastic/elasticsearch/issues/122548 # Examples: # diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchTimeoutIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchTimeoutIT.java index f79321ef8d0de..8100e911f77cb 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/SearchTimeoutIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/SearchTimeoutIT.java @@ -484,8 +484,7 @@ protected TermSuggestion innerExecute( CharsRefBuilder spare ) { contextIndexSearcher.throwTimeExceededException(); - assert false; - return new TermSuggestion(name, suggestion.getSize(), SortBy.SCORE); + throw new AssertionError("should have thrown TimeExceededException"); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/query/QueryPhase.java b/server/src/main/java/org/elasticsearch/search/query/QueryPhase.java index 8ad52c4f9bb59..5fcfb2b9766cd 100644 --- a/server/src/main/java/org/elasticsearch/search/query/QueryPhase.java +++ b/server/src/main/java/org/elasticsearch/search/query/QueryPhase.java @@ -126,15 +126,7 @@ static void executeRank(SearchContext searchContext) throws QueryPhaseExecutionE static void executeQuery(SearchContext searchContext) throws QueryPhaseExecutionException { if (searchContext.hasOnlySuggest()) { - try { - SuggestPhase.execute(searchContext); - } catch (ContextIndexSearcher.TimeExceededException timeExceededException) { - SearchTimeoutException.handleTimeout( - searchContext.request().allowPartialSearchResults(), - searchContext.shardTarget(), - searchContext.queryResult() - ); - } + SuggestPhase.execute(searchContext); searchContext.queryResult().topDocs(new TopDocsAndMaxScore(Lucene.EMPTY_TOP_DOCS, Float.NaN), new DocValueFormat[0]); return; } @@ -150,18 +142,10 @@ static void executeQuery(SearchContext searchContext) throws QueryPhaseExecution addCollectorsAndSearch(searchContext); - try { - RescorePhase.execute(searchContext); - SuggestPhase.execute(searchContext); - if (searchContext.getProfilers() != null) { - searchContext.queryResult().profileResults(searchContext.getProfilers().buildQueryPhaseResults()); - } - } catch (ContextIndexSearcher.TimeExceededException timeExceededException) { - SearchTimeoutException.handleTimeout( - searchContext.request().allowPartialSearchResults(), - searchContext.shardTarget(), - searchContext.queryResult() - ); + RescorePhase.execute(searchContext); + SuggestPhase.execute(searchContext); + if (searchContext.getProfilers() != null) { + searchContext.queryResult().profileResults(searchContext.getProfilers().buildQueryPhaseResults()); } } diff --git a/server/src/main/java/org/elasticsearch/search/rescore/RescorePhase.java b/server/src/main/java/org/elasticsearch/search/rescore/RescorePhase.java index 7223da3c6101b..fdd5efceaae3c 100644 --- a/server/src/main/java/org/elasticsearch/search/rescore/RescorePhase.java +++ b/server/src/main/java/org/elasticsearch/search/rescore/RescorePhase.java @@ -18,7 +18,9 @@ import org.elasticsearch.common.lucene.search.TopDocsAndMaxScore; import org.elasticsearch.common.util.Maps; import org.elasticsearch.lucene.grouping.TopFieldGroups; +import org.elasticsearch.search.internal.ContextIndexSearcher; import org.elasticsearch.search.internal.SearchContext; +import org.elasticsearch.search.query.SearchTimeoutException; import org.elasticsearch.search.sort.ShardDocSortField; import org.elasticsearch.search.sort.SortAndFormats; @@ -84,6 +86,13 @@ public static void execute(SearchContext context) { .topDocs(new TopDocsAndMaxScore(topDocs, topDocs.scoreDocs[0].score), context.queryResult().sortValueFormats()); } catch (IOException e) { throw new ElasticsearchException("Rescore Phase Failed", e); + } catch (ContextIndexSearcher.TimeExceededException timeExceededException) { + SearchTimeoutException.handleTimeout( + context.request().allowPartialSearchResults(), + context.shardTarget(), + context.queryResult() + ); + // if the rescore phase times out and partial results are allowed, the returned top docs from this shard won't be rescored } } diff --git a/server/src/main/java/org/elasticsearch/search/suggest/SuggestPhase.java b/server/src/main/java/org/elasticsearch/search/suggest/SuggestPhase.java index 272855bacd544..17ff07f167ff8 100644 --- a/server/src/main/java/org/elasticsearch/search/suggest/SuggestPhase.java +++ b/server/src/main/java/org/elasticsearch/search/suggest/SuggestPhase.java @@ -10,7 +10,9 @@ import org.apache.lucene.util.CharsRefBuilder; import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.search.internal.ContextIndexSearcher; import org.elasticsearch.search.internal.SearchContext; +import org.elasticsearch.search.query.SearchTimeoutException; import org.elasticsearch.search.suggest.Suggest.Suggestion; import org.elasticsearch.search.suggest.Suggest.Suggestion.Entry; import org.elasticsearch.search.suggest.Suggest.Suggestion.Entry.Option; @@ -40,12 +42,17 @@ public static void execute(SearchContext context) { for (Map.Entry entry : suggest.suggestions().entrySet()) { SuggestionSearchContext.SuggestionContext suggestion = entry.getValue(); Suggester suggester = suggestion.getSuggester(); - Suggestion> result = suggester.execute( - entry.getKey(), - suggestion, - context.searcher(), - spare - ); + Suggestion> result; + try { + result = suggester.execute(entry.getKey(), suggestion, context.searcher(), spare); + } catch (ContextIndexSearcher.TimeExceededException timeExceededException) { + SearchTimeoutException.handleTimeout( + context.request().allowPartialSearchResults(), + context.shardTarget(), + context.queryResult() + ); + result = suggester.emptySuggestion(entry.getKey(), suggestion, spare); + } if (result != null) { assert entry.getKey().equals(result.name); suggestions.add(result); diff --git a/server/src/test/java/org/elasticsearch/search/query/QueryPhaseTimeoutTests.java b/server/src/test/java/org/elasticsearch/search/query/QueryPhaseTimeoutTests.java index 6b38e05bdc4e3..aa9ca26974bad 100644 --- a/server/src/test/java/org/elasticsearch/search/query/QueryPhaseTimeoutTests.java +++ b/server/src/test/java/org/elasticsearch/search/query/QueryPhaseTimeoutTests.java @@ -297,7 +297,8 @@ public void testSuggestOnlyWithTimeout() throws Exception { assertTrue(context.hasOnlySuggest()); QueryPhase.execute(context); assertTrue(context.queryResult().searchTimedOut()); - assertNull(context.queryResult().suggest()); + assertEquals(1, context.queryResult().suggest().size()); + assertEquals(0, context.queryResult().suggest().getSuggestion("suggestion").getEntries().size()); assertNotNull(context.queryResult().topDocs()); assertEquals(0, context.queryResult().topDocs().topDocs.totalHits.value()); } @@ -311,7 +312,8 @@ public void testSuggestAndQueryWithSuggestTimeout() throws Exception { QueryPhase.execute(context); assertThat(context.queryResult().topDocs().topDocs.totalHits.value(), Matchers.greaterThan(0L)); assertTrue(context.queryResult().searchTimedOut()); - assertNull(context.queryResult().suggest()); + assertEquals(1, context.queryResult().suggest().size()); + assertEquals(0, context.queryResult().suggest().getSuggestion("suggestion").getEntries().size()); } }