From 7af2fec66695b7407f82fe57ca58c8a96fcd077f Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Wed, 9 Sep 2020 11:43:13 -0400 Subject: [PATCH 1/4] Fix testInvalidScrollKeepAlive --- .../search/scroll/SearchScrollIT.java | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/scroll/SearchScrollIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/scroll/SearchScrollIT.java index f26c160895706..cefc6d3d81c4b 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/scroll/SearchScrollIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/scroll/SearchScrollIT.java @@ -610,16 +610,19 @@ public void testInvalidScrollKeepAlive() throws IOException { .setScroll(TimeValue.timeValueMinutes(5)) .get(); assertNotNull(searchResponse.getScrollId()); - assertThat(searchResponse.getHits().getTotalHits().value, equalTo(2L)); - assertThat(searchResponse.getHits().getHits().length, equalTo(1)); + try { + assertThat(searchResponse.getHits().getTotalHits().value, equalTo(2L)); + assertThat(searchResponse.getHits().getHits().length, equalTo(1)); - exc = expectThrows(Exception.class, - () -> client().prepareSearchScroll(searchResponse.getScrollId()) - .setScroll(TimeValue.timeValueHours(3)).get()); - illegalArgumentException = - (IllegalArgumentException) ExceptionsHelper.unwrap(exc, IllegalArgumentException.class); - assertNotNull(illegalArgumentException); - assertThat(illegalArgumentException.getMessage(), containsString("Keep alive for request (3h) is too large")); + exc = expectThrows(Exception.class, + () -> client().prepareSearchScroll(searchResponse.getScrollId()).setScroll(TimeValue.timeValueHours(3)).get()); + illegalArgumentException = + (IllegalArgumentException) ExceptionsHelper.unwrap(exc, IllegalArgumentException.class); + assertNotNull(illegalArgumentException); + assertThat(illegalArgumentException.getMessage(), containsString("Keep alive for request (3h) is too large")); + } finally { + client().prepareClearScroll().addScrollId(searchResponse.getScrollId()).get(); + } } /** From 118cd0e6e22816140d418a4555259bffba206435 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Wed, 9 Sep 2020 12:11:14 -0400 Subject: [PATCH 2/4] Revert "Fix testInvalidScrollKeepAlive" This reverts commit 7af2fec66695b7407f82fe57ca58c8a96fcd077f. --- .../search/scroll/SearchScrollIT.java | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/scroll/SearchScrollIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/scroll/SearchScrollIT.java index cefc6d3d81c4b..f26c160895706 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/scroll/SearchScrollIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/scroll/SearchScrollIT.java @@ -610,19 +610,16 @@ public void testInvalidScrollKeepAlive() throws IOException { .setScroll(TimeValue.timeValueMinutes(5)) .get(); assertNotNull(searchResponse.getScrollId()); - try { - assertThat(searchResponse.getHits().getTotalHits().value, equalTo(2L)); - assertThat(searchResponse.getHits().getHits().length, equalTo(1)); + assertThat(searchResponse.getHits().getTotalHits().value, equalTo(2L)); + assertThat(searchResponse.getHits().getHits().length, equalTo(1)); - exc = expectThrows(Exception.class, - () -> client().prepareSearchScroll(searchResponse.getScrollId()).setScroll(TimeValue.timeValueHours(3)).get()); - illegalArgumentException = - (IllegalArgumentException) ExceptionsHelper.unwrap(exc, IllegalArgumentException.class); - assertNotNull(illegalArgumentException); - assertThat(illegalArgumentException.getMessage(), containsString("Keep alive for request (3h) is too large")); - } finally { - client().prepareClearScroll().addScrollId(searchResponse.getScrollId()).get(); - } + exc = expectThrows(Exception.class, + () -> client().prepareSearchScroll(searchResponse.getScrollId()) + .setScroll(TimeValue.timeValueHours(3)).get()); + illegalArgumentException = + (IllegalArgumentException) ExceptionsHelper.unwrap(exc, IllegalArgumentException.class); + assertNotNull(illegalArgumentException); + assertThat(illegalArgumentException.getMessage(), containsString("Keep alive for request (3h) is too large")); } /** From ce109e513e33c8a7a014ac23824d1a060b1adcee Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Wed, 9 Sep 2020 12:17:12 -0400 Subject: [PATCH 3/4] Release scroll context --- .../elasticsearch/search/SearchService.java | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/SearchService.java b/server/src/main/java/org/elasticsearch/search/SearchService.java index 2de2497aacfbb..389f6472354c0 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchService.java +++ b/server/src/main/java/org/elasticsearch/search/SearchService.java @@ -471,7 +471,14 @@ public void executeQueryPhase(InternalScrollSearchRequest request, SearchShardTask task, ActionListener listener) { final LegacyReaderContext readerContext = (LegacyReaderContext) findReaderContext(request.contextId(), request); - final Releasable markAsUsed = readerContext.markAsUsed(getScrollKeepAlive(request.scroll())); + final Releasable markAsUsed; + try { + markAsUsed = readerContext.markAsUsed(getScrollKeepAlive(request.scroll())); + } catch (Exception e) { + // We need to release the reader context of the scroll when we hit any exception (here the keep_alive can be too large) + processFailure(readerContext, e); + throw e; + } runAsync(getExecutor(readerContext.indexShard()), () -> { final ShardSearchRequest shardSearchRequest = readerContext.getShardSearchRequest(null); try (SearchContext searchContext = createContext(readerContext, shardSearchRequest, task, false); @@ -536,7 +543,14 @@ private Executor getExecutor(IndexShard indexShard) { public void executeFetchPhase(InternalScrollSearchRequest request, SearchShardTask task, ActionListener listener) { final LegacyReaderContext readerContext = (LegacyReaderContext) findReaderContext(request.contextId(), request); - final Releasable markAsUsed = readerContext.markAsUsed(getScrollKeepAlive(request.scroll())); + final Releasable markAsUsed; + try { + markAsUsed = readerContext.markAsUsed(getScrollKeepAlive(request.scroll())); + } catch (Exception e) { + // We need to release the reader context of the scroll when we hit any exception (here the keep_alive can be too large) + processFailure(readerContext, e); + throw e; + } runAsync(getExecutor(readerContext.indexShard()), () -> { final ShardSearchRequest shardSearchRequest = readerContext.getShardSearchRequest(null); try (SearchContext searchContext = createContext(readerContext, shardSearchRequest, task, false); From 3517cb56f1130288b92b9587e65e267fa6873626 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Wed, 9 Sep 2020 12:25:27 -0400 Subject: [PATCH 4/4] feedback --- .../src/main/java/org/elasticsearch/search/SearchService.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/SearchService.java b/server/src/main/java/org/elasticsearch/search/SearchService.java index 389f6472354c0..f716eb61707a3 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchService.java +++ b/server/src/main/java/org/elasticsearch/search/SearchService.java @@ -476,7 +476,7 @@ public void executeQueryPhase(InternalScrollSearchRequest request, markAsUsed = readerContext.markAsUsed(getScrollKeepAlive(request.scroll())); } catch (Exception e) { // We need to release the reader context of the scroll when we hit any exception (here the keep_alive can be too large) - processFailure(readerContext, e); + freeReaderContext(readerContext.id()); throw e; } runAsync(getExecutor(readerContext.indexShard()), () -> { @@ -548,7 +548,7 @@ public void executeFetchPhase(InternalScrollSearchRequest request, SearchShardTa markAsUsed = readerContext.markAsUsed(getScrollKeepAlive(request.scroll())); } catch (Exception e) { // We need to release the reader context of the scroll when we hit any exception (here the keep_alive can be too large) - processFailure(readerContext, e); + freeReaderContext(readerContext.id()); throw e; } runAsync(getExecutor(readerContext.indexShard()), () -> {