Implement Cancellable DirectoryReader #52822

matriv · 2020-02-26T15:01:43Z

Implement a Cancellable DirectoryReader that wraps the original
DirectoryReader so that when a search task is cancelled the
DirectoryReaders also stop their work fast. This is usuful for
expensive operations like wilcard/prefix queries where the
DirectoryReaders can spend lots of time and consume resources,
as previously their work wouldn't stop even though the original
search task was cancelled (e.g. because of timeout or dropped client
connection).

Implement a Cancellable DirectoryReader that wraps the original DirectoryReader so that when a search task is cancelled the DirectoryReaders also stop their work fast. This is usuful for expensive operations like wilcard/prefix queries where the DirectoryReaders can spend lots of time and consume resources, as previously their work wouldn't stop even though the original search task was cancelled (e.g. because of timeout or dropped client connection).

elasticmachine · 2020-02-26T15:01:45Z

Pinging @elastic/es-search (:Search/Search)

jpountz

I left some suggestions but this looks like it's on the right track to me.

jpountz · 2020-02-26T15:10:02Z

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

@@ -708,6 +708,8 @@ private DefaultSearchContext createSearchContext(SearchRewriteContext rewriteCon
                searcher, clusterService, indexService, indexShard, bigArrays, threadPool::relativeTimeInMillis, timeout, fetchPhase);
            success = true;
            return searchContext;
+        } catch (IOException e) {
+            return null;


this leniency looks dangerous?

Should I simply propagate the exception? or any other suggestion?

jpountz · 2020-02-26T15:13:21Z

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

+            }
+            cancellable.get().run();
+            return false;
+        }


This implementation feels a bit awkward, I'd rather like to fork ExitableDirectoryReader entirely to not inherit from its QueryTimeout abstraction.

I tried that in the 1st approach but this means we have to copy the whole ExitablePointValues to wrap the point values and therefore the ExitableIntersectVisitor.

I don't mind copying it.

jpountz · 2020-02-26T15:15:24Z

...ava/org/elasticsearch/search/aggregations/metrics/TDigestPercentileRanksAggregatorTests.java

            IndexSearcher searcher = new IndexSearcher(reader);
            PercentileRanks ranks = search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType);
            Percentile rank = ranks.iterator().next();
            assertEquals(Double.NaN, rank.getPercent(), 0d);
            assertEquals(0.5, rank.getValue(), 0d);
            assertFalse(AggregationInspectionHelper.hasValue(((InternalTDigestPercentileRanks)ranks)));
+        } finally {
+            unmappedIndexWriter.close();
+            directory.close();


use try-with-resources?

jpountz · 2020-02-26T15:17:49Z

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

+    public ContextIndexSearcher(IndexReader reader, Similarity similarity,
+                                QueryCache queryCache, QueryCachingPolicy queryCachingPolicy,
+                                boolean shouldWrap) throws IOException {
+        super(shouldWrap? new CancellableIndexReader((DirectoryReader) reader, new Cancellable()) : reader);


CancellableIndexReader shouldn't have any overhead, so it might be simpler to wrap all the time here.

This boolean was added together with a second constructor because of the AggregatorTestCase and hundreds of tests that derive from that. If we wrap the IndexReader we get:

java.lang.AssertionError: The top-reader used to create Weight is not the same as the current reader's top-reader (org.apache.lucene.index.CompositeReaderContext@382edaaa

which I tried to fix by changing the AggregatorTestCase to receive IndexReader and not IndexSearcher as an argument. and all the tests to use the derived IndexSearcher from the context created. But even with this there were a few more tests failing that didn't manage to fix, so after discussion with @jimczi we decided to make this workaround for the moment and address the issue in a separate PR afterwards.

I can add a TODO though to not miss it.

jpountz · 2020-02-26T15:18:26Z

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

+    /**
+     * Wraps an {@link IndexReader} with a cancellation Runnable task.
+     */
+    private static class CancellableIndexReader extends FilterDirectoryReader {


Let's call it CancellableDirectoryReader if it extends DirectoryReader.

matriv · 2020-02-27T22:39:52Z

@jpountz @jimczi I had to fork the ExitableDirectoryReader because of this: https://github.com/elastic/elasticsearch/pull/52822/files#diff-913ad694a7d744ee93ae1dac48d67b0eR415

The CompletionTerms is a final class and the code in CompletionWeight here: https://github.com/apache/lucene-solr/blob/master/lucene/suggest/src/java/org/apache/lucene/search/suggest/document/CompletionWeight.java#L76 expects CompletionTerms, so if they are wrapped into ExitableTerms the if leads to an IllegalArguementException.

jpountz

Forking sounds totally fine to me.

Can you explain why we need to unset the timeout at some point? I'm guessing that it is to allow aggregations to build their results without errors? A comment would help clarify.

Also I'm not sure why some tests need to become integration tests? These tests used to be integration tests that we migrated to unit test.

jpountz · 2020-02-28T09:00:20Z

server/src/main/java/org/elasticsearch/search/query/QueryPhase.java

@@ -310,6 +297,8 @@ static boolean executeInternal(SearchContext searchContext) throws QueryPhaseExe
                queryResult.nodeQueueSize(rExecutor.getCurrentQueueSize());
                queryResult.serviceTimeEWMA((long) rExecutor.getTaskExecutionEWMA());
            }
+            // Search phase has finished, no longer need to check for timeout
+            searcher.unsetCheckTimeout();


Why do we need to do this?

So that we don't experience a timeout during the aggregation phase. (there were integ tests that were failing because of this). I changed the comment to mention it.

We should do this on the cancellable object directly. The searcher shouldn't be able to unset this way. See my comment on the context searcher.

matriv · 2020-02-28T13:11:52Z

Forking sounds totally fine to me.

Thx!

Also I'm not sure why some tests need to become integration tests? These tests used to be integration tests that we migrated to unit test.

@jimczi Could you please help explaining?
Since we have at the moment that 2nd constructor that's necessary for the AggregatorTestCase tests maybe we can use it also for those so we don't attempt to wrap the MultiReader.

jimczi · 2020-02-28T13:25:39Z

Also I'm not sure why some tests need to become integration tests? These tests used to be integration tests that we migrated to unit test.

We can probably fix these tests rather than moving them again into an integration tests. We've made some workaround in the AggregatorTestCase that are now hitting us badly. For instance we allow to use a MultiReader instead of a DirectoryReader so a lot of new tests are using this shortcut even though we always expect a DirectoryReader when used outside of tests. We also have a unit test that simulates the mixing of an unmapped field with a mapped one but the base class AggregatorTestCase only exposes a single mapping. I think we should cleanup those tests and ensure that we're making the same assumption in tests than when deploying, however this is out of scope for this pr so I told Marios that we should tackle this in a separate one. To unblock his work we have the workaround here so that we don't try to wrap the reader if it's not a standard directory reader.

jimczi

It looks good to me overall. I left some comments to simplify the logic in the ContextIndexSearcher and to move the cancellation impl in the QueryPhase directly.

jimczi · 2020-02-28T15:20:00Z

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

+        void unsetCheckTimeout();
+    }
+
+    public static class CancellableImpl implements Cancellable {


The implementation should live on the QueryPhase imo. This way you don't need to expose the checkDirReaderCancelled and checkDirReaderCancelled in the searcher.

jimczi · 2020-02-28T15:20:38Z

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

+
+        boolean isEnabled();
+        void checkCancelled();
+        default void checkDirReaderCancelled() {


These two functions should not be part of the interface, checkCancelled should be enough

I don't like that much either, I exposed the checkDirReaderCancelled() so that I we can have tests that differentiate between exit in the searcher and exit from the dirReader: https://github.com/elastic/elasticsearch/pull/52822/files#diff-2abce48f3a52657ce3740afade6d5f8fR132
But as discussed I could just unit test the Cancellable reader instead.

jimczi · 2020-02-28T15:20:58Z

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

+    /**
+     * iFace which implements the query timeout / cancellation logic
+     */
+    public interface Cancellable {


Can you call QueryCancellable ?

jimczi · 2020-02-28T15:24:34Z

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

@@ -77,10 +89,27 @@

    private AggregatedDfs aggregatedDfs;
    private QueryProfiler profiler;
-    private Runnable checkCancelled;
+    private Holder<Cancellable> cancellable;


why do you need to use a custom Holder ? Now that cancellable directory reader is forked, you should be able to set the cancellable lazily ? So instead of passing the Cancellable here you'd set it in ContextIndexSearcher#setCancellable ?

The issue is that we need a reference to the cancellable (the holder actually) during construction of the Reader: https://github.com/elastic/elasticsearch/pull/52822/files#diff-913ad694a7d744ee93ae1dac48d67b0eR403
So that later on with the setCancellable we can set it to the holder.

Alreanatively I changed so that the ContextIndexSearcher doesn't know of the holder, but then a cast to CancellableDirectoryReader is required to call the setCancellable.

jimczi · 2020-02-28T15:25:23Z

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

+            }
+            cancellable.get().run();
+            return false;
+        }


jimczi · 2020-02-28T15:26:10Z

server/src/main/java/org/elasticsearch/search/query/QueryPhase.java

@@ -310,6 +297,8 @@ static boolean executeInternal(SearchContext searchContext) throws QueryPhaseExe
                queryResult.nodeQueueSize(rExecutor.getCurrentQueueSize());
                queryResult.serviceTimeEWMA((long) rExecutor.getTaskExecutionEWMA());
            }
+            // Search phase has finished, no longer need to check for timeout
+            searcher.unsetCheckTimeout();


We should do this on the cancellable object directly. The searcher shouldn't be able to unset this way. See my comment on the context searcher.

jimczi · 2020-02-28T15:27:11Z

server/src/test/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorTests.java

@@ -611,14 +611,8 @@ public void testCacheAggregation() throws IOException {
        }


Can you revert this change ? It should work without this modification so I'd like to keep this for a different pr since the issue is not related to the exitable directory reader.

Sure, will do.

jimczi · 2020-02-28T15:27:37Z

server/src/test/java/org/elasticsearch/search/aggregations/metrics/MaxAggregatorTests.java

@@ -920,14 +920,8 @@ public void testCacheAggregation() throws IOException {
        }


Same here, this shouldn't be needed with the extra constructor.

jimczi · 2020-02-28T15:27:48Z

...ava/org/elasticsearch/search/aggregations/metrics/TDigestPercentileRanksAggregatorTests.java

@@ -22,7 +22,6 @@
 import org.apache.lucene.document.Document;


Here too ;)

jpountz

Some additional to @jimczi 's.

jpountz · 2020-02-28T16:54:41Z

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

-    public void setCheckCancelled(Runnable checkCancelled) {
-        this.checkCancelled = checkCancelled;
+    public void setCancellable(Cancellable cancellable) {
+        this.cancellable.set(cancellable);


can you check that the argument is not null since we seem to assume it's never null in some places?

jpountz · 2020-02-28T16:56:12Z

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

@@ -244,7 +270,7 @@ public Scorer scorer(LeafReaderContext context) throws IOException {
                public BulkScorer bulkScorer(LeafReaderContext context) throws IOException {
                    BulkScorer in = weight.bulkScorer(context);
                    if (in != null) {
-                        return new CancellableBulkScorer(in, checkCancelled);
+                        return new CancellableBulkScorer(in, () -> cancellable.get().checkCancelled());


Let's do the get() call once for all instead of forcing the bulk scorer to do it every time it wants to check whether the request has been cancelled?

Suggested change

return new CancellableBulkScorer(in, () -> cancellable.get().checkCancelled());

Runnable checkCancelled = cancellable.get();

return new CancellableBulkScorer(in, checkCancelled);

jpountz · 2020-02-28T16:58:48Z

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

+
+        @Override
+        protected DirectoryReader doWrapDirectoryReader(DirectoryReader in) {
+            return in;


let's throw an UnsupportedOperationException? (this is only used when asking a DirectoryReader to take into account some new changes in a directory, which should never happen with this impl)

jpountz · 2020-02-28T17:01:20Z

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

+        }
+
+        private void checkAndThrowWithSampling() {
+            if (calls++ % MAX_CALLS_BEFORE_QUERY_TIMEOUT_CHECK == 0) {


If we want to reduce the overhead to a minimum, we could make MAX_CALLS_BEFORE_QUERY_TIMEOUT_CHECK a power of two minus 1 (e.g. (1 << 4) - 1, ie. 15, and use a mask instead of a remainder: (calls++ & MAX_CALLS_BEFORE_QUERY_TIMEOUT_CHECK) == 0.

Could you please explain why power of two - 1 is better?

Masks are typically cheaper than remainders. And a%b is the same as a & (b-1) when a is positive and b is a power of two.

jimczi

Thanks for all the iterations @matriv, LGTM!

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

jimczi · 2020-03-05T10:34:50Z

server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java

    }

    public void setProfiler(QueryProfiler profiler) {
        this.profiler = profiler;
    }

    /**
-     * Set a {@link Runnable} that will be run on a regular basis while
-     * collecting documents.
+     * Add a {@link Runnable} that will be run on a regular basis while fetching document from the


nit: s/while accessing/while fetching document/ this is confusing since we don't check the cancellation when visiting the stored fields ?

matriv · 2020-03-05T10:41:39Z

@jimczi Thx you too for you patience and all the help and guidance you provided.

Implement an Exitable DirectoryReader that wraps the original DirectoryReader so that when a search task is cancelled the DirectoryReaders also stop their work fast. This is usuful for expensive operations like wilcard/prefix queries where the DirectoryReaders can spend lots of time and consume resources, as previously their work wouldn't stop even though the original search task was cancelled (e.g. because of timeout or dropped client connection). (cherry picked from commit 67acaf6)

With ExitableDirectoryReader in place, check for query cancellation during QueryPhase#preProcess where the query rewriting takes place. Follows: elastic#52822

With ExitableDirectoryReader in place, check for query cancellation during QueryPhase#preProcess where the query rewriting takes place. Follows: #52822

With ExitableDirectoryReader in place, check for query cancellation during QueryPhase#preProcess where the query rewriting takes place. Follows: #52822 (cherry picked from commit 0d38626)

The introduction of the ExitableDirectoryReader showed increase of latencies for range queries using pointvalues. Check for cancellation every 1024 docs instead of every 15 to lower the impact of the check in query's performance. Follows: #52822 Fixes: #53496

The introduction of the ExitableDirectoryReader showed increase of latencies for range queries using pointvalues. Check for cancellation every 1024 docs instead of every 15 to lower the impact of the check in query's performance. Follows: #52822 Fixes: #53496 (cherry picked from commit 6b5fc35)

Benchmarking showed that the effect of the ExitableDirectoryReader is reduced considerably when checking every 4095 docs. Moreover, set the cancellable task before calling QueryPhase.preProcess() and make sure we don't wrap with an ExitableDirectoryReader at all when lowLevelCancellation() is set to false to avoid completely any performance impact. Follows: elastic#52822 Follows: elastic#53166 Follows: elastic#53496

Benchmarking showed that the effect of the ExitableDirectoryReader is reduced considerably when checking every 8191 docs. Moreover, set the cancellable task before calling QueryPhase#preProcess() and make sure we don't wrap with an ExitableDirectoryReader at all when lowLevelCancellation is set to false to avoid completely any performance impact. Follows: #52822 Follows: #53166 Follows: #53496

Benchmarking showed that the effect of the ExitableDirectoryReader is reduced considerably when checking every 8191 docs. Moreover, set the cancellable task before calling QueryPhase#preProcess() and make sure we don't wrap with an ExitableDirectoryReader at all when lowLevelCancellation is set to false to avoid completely any performance impact. Follows: #52822 Follows: #53166 Follows: #53496 (cherry picked from commit cdc377e)

matriv added >enhancement :Search/Search Search-related issues that do not fall into other categories labels Feb 26, 2020

jpountz self-requested a review February 26, 2020 15:02

jpountz reviewed Feb 26, 2020

View reviewed changes

matriv and others added 11 commits February 26, 2020 16:49

fix compilation

c890142

Merge remote-tracking branch 'upstream/master' into impl-cancellable

e38cfa0

Address comments

73b0e6d

Fix behaviour by properly handling cancellable.run()

ffdf6d2

split query timeout and cancellation to be able to unset query timeout

add7dd4

Merge remote-tracking branch 'upstream/master' into impl-cancellable

d74edb2

Custom implementation of ExitableDirReader to overcome casting

331411b

revert changes

d10c51a

revert

e5fdf47

revert unrelated changes

248ee51

Merge remote-tracking branch 'upstream/master' into impl-cancellable

bc85193

Fix issues after splitting to cancelation and timeout

ce2d557

jpountz reviewed Feb 28, 2020

View reviewed changes

Add unit test

0012e3a

matriv added 3 commits February 28, 2020 14:59

re-enable MultiReader Tests, fix cancellation tests

4c3183f

fix checkstyle - enhance comments

8b38977

Fix NPE

9ebd847

matriv marked this pull request as ready for review February 28, 2020 15:14

matriv requested a review from jimczi February 28, 2020 15:14

jimczi requested changes Feb 28, 2020

View reviewed changes

jpountz reviewed Feb 28, 2020

View reviewed changes

jimczi approved these changes Mar 5, 2020

View reviewed changes

fix comment

eb158e5

matriv merged commit 67acaf6 into elastic:master Mar 5, 2020

matriv deleted the impl-cancellable branch March 5, 2020 11:50

matriv mentioned this pull request Mar 5, 2020

Check for query cancellation during rewrite #53166

Merged

matriv added a commit that referenced this pull request Mar 5, 2020

Check for query cancellation during rewrite (#53166)

0d38626

With ExitableDirectoryReader in place, check for query cancellation during QueryPhase#preProcess where the query rewriting takes place. Follows: #52822

matriv mentioned this pull request Mar 5, 2020

Check for query cancellation during rewrite (#53166) #53203

Merged

jimczi mentioned this pull request Mar 12, 2020

ExitableDirectoryReader slowdowns intensive points and terms query #53496

Closed

markharwood mentioned this pull request Mar 13, 2020

Range field performance regression after Lucene compression change #53498

Closed

matriv mentioned this pull request Mar 18, 2020

Increase step between checks for cancellation #53712

Merged

matriv mentioned this pull request Mar 23, 2020

Reduce performance impact of ExitableDirectoryReader #53978

Merged

matriv mentioned this pull request Mar 23, 2020

Reduce performance impact of ExitableDirectoryReader (#53978) #54014

Merged

This was referenced Apr 1, 2020

7.7.0 meta ticket elastic/elasticsearch-net#4525

Closed

7.7.0 meta ticket (Part 2) elastic/elasticsearch-net#4533

Closed

jakelandis mentioned this pull request Dec 2, 2020

Very large scroll search (i.e. reindex) can gradually slow down #65780

Closed

dnhatn mentioned this pull request Mar 16, 2021

Automatic task cancellation mechanism is invalid on coordinating node's reduce phase #70347

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

		@@ -611,14 +611,8 @@ public void testCacheAggregation() throws IOException {
		}

		@@ -920,14 +920,8 @@ public void testCacheAggregation() throws IOException {
		}

		@@ -22,7 +22,6 @@
		import org.apache.lucene.document.Document;

	return new CancellableBulkScorer(in, () -> cancellable.get().checkCancelled());
	Runnable checkCancelled = cancellable.get();
	return new CancellableBulkScorer(in, checkCancelled);

Implement Cancellable DirectoryReader #52822

Implement Cancellable DirectoryReader #52822

Conversation

matriv commented Feb 26, 2020

elasticmachine commented Feb 26, 2020

jpountz left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Feb 27, 2020

jpountz left a 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

matriv commented Feb 28, 2020 • edited Loading

jimczi commented Feb 28, 2020

jimczi left a 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

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

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

jpountz left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpountz Mar 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Mar 5, 2020

matriv commented Feb 28, 2020 •

edited

Loading

jpountz Mar 2, 2020 •

edited

Loading