-
Notifications
You must be signed in to change notification settings - Fork 25k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Cancellable DirectoryReader #52822
Changes from 1 commit
e2ebfb4
c890142
e38cfa0
73b0e6d
ffdf6d2
add7dd4
d74edb2
331411b
d10c51a
e5fdf47
248ee51
bc85193
ce2d557
0012e3a
4c3183f
8b38977
9ebd847
0bf64f0
3936a05
9bf0fe3
ce51935
9695114
d562cf1
6243ced
19bdbdf
b446dfd
183da17
a912fa3
23c3adc
087f2ad
df0da4c
eb158e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,9 +19,18 @@ | |
|
||
package org.elasticsearch.search.internal; | ||
|
||
import org.apache.lucene.index.BinaryDocValues; | ||
import org.apache.lucene.index.DirectoryReader; | ||
import org.apache.lucene.index.ExitableDirectoryReader; | ||
import org.apache.lucene.index.FilterDirectoryReader; | ||
import org.apache.lucene.index.IndexReader; | ||
import org.apache.lucene.index.LeafReader; | ||
import org.apache.lucene.index.LeafReaderContext; | ||
import org.apache.lucene.index.NumericDocValues; | ||
import org.apache.lucene.index.QueryTimeout; | ||
import org.apache.lucene.index.SortedDocValues; | ||
import org.apache.lucene.index.SortedNumericDocValues; | ||
import org.apache.lucene.index.SortedSetDocValues; | ||
import org.apache.lucene.index.Term; | ||
import org.apache.lucene.search.BulkScorer; | ||
import org.apache.lucene.search.CollectionStatistics; | ||
|
@@ -64,6 +73,7 @@ | |
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.Set; | ||
import java.util.function.Supplier; | ||
|
||
/** | ||
* Context-aware extension of {@link IndexSearcher}. | ||
|
@@ -79,8 +89,18 @@ public class ContextIndexSearcher extends IndexSearcher { | |
private QueryProfiler profiler; | ||
private Runnable checkCancelled; | ||
|
||
public ContextIndexSearcher(IndexReader reader, Similarity similarity, QueryCache queryCache, QueryCachingPolicy queryCachingPolicy) { | ||
super(reader); | ||
public ContextIndexSearcher(IndexReader reader, Similarity similarity, | ||
QueryCache queryCache, QueryCachingPolicy queryCachingPolicy) throws IOException { | ||
this(reader, similarity, queryCache, queryCachingPolicy, true); | ||
} | ||
|
||
public ContextIndexSearcher(IndexReader reader, Similarity similarity, | ||
QueryCache queryCache, QueryCachingPolicy queryCachingPolicy, | ||
boolean shouldWrap) throws IOException { | ||
super(shouldWrap? new CancellableIndexReader((DirectoryReader) reader, new Cancellable()) : reader); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CancellableIndexReader shouldn't have any overhead, so it might be simpler to wrap all the time here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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. |
||
if (shouldWrap) { | ||
((CancellableIndexReader) getIndexReader()).setCheckCancelled(() -> checkCancelled); | ||
} | ||
setSimilarity(similarity); | ||
setQueryCache(queryCache); | ||
setQueryCachingPolicy(queryCachingPolicy); | ||
|
@@ -320,4 +340,99 @@ public DirectoryReader getDirectoryReader() { | |
assert reader instanceof DirectoryReader : "expected an instance of DirectoryReader, got " + reader.getClass(); | ||
return (DirectoryReader) reader; | ||
} | ||
|
||
/** | ||
* Wraps an {@link IndexReader} with a cancellation Runnable task. | ||
*/ | ||
private static class CancellableIndexReader extends FilterDirectoryReader { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's call it |
||
|
||
private final Cancellable checkCancelled; | ||
|
||
private CancellableIndexReader(DirectoryReader in, Cancellable checkCancelled) throws IOException { | ||
super(in, new SubReaderWrapper() { | ||
@Override | ||
public LeafReader wrap(LeafReader reader) { | ||
return new CancellableLeafReader(reader, checkCancelled); | ||
} | ||
}); | ||
this.checkCancelled = checkCancelled; | ||
} | ||
|
||
private void setCheckCancelled(Supplier<Runnable> checkCancelled) { | ||
this.checkCancelled.setCancellable(checkCancelled); | ||
} | ||
|
||
@Override | ||
protected DirectoryReader doWrapDirectoryReader(DirectoryReader in) { | ||
return in; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
} | ||
|
||
@Override | ||
public CacheHelper getReaderCacheHelper() { | ||
return in.getReaderCacheHelper(); | ||
} | ||
} | ||
|
||
/** | ||
* Wraps a leaf reader with a cancellable task | ||
*/ | ||
private static class CancellableLeafReader extends ExitableDirectoryReader.ExitableFilterAtomicReader { | ||
|
||
private CancellableLeafReader(LeafReader leafReader, Cancellable checkCancelled) { | ||
super(leafReader, checkCancelled); | ||
} | ||
|
||
@Override | ||
public NumericDocValues getNumericDocValues(String field) throws IOException { | ||
return in.getNumericDocValues(field); | ||
} | ||
|
||
@Override | ||
public BinaryDocValues getBinaryDocValues(String field) throws IOException { | ||
return in.getBinaryDocValues(field); | ||
} | ||
|
||
@Override | ||
public SortedDocValues getSortedDocValues(String field) throws IOException { | ||
return in.getSortedDocValues(field); | ||
} | ||
|
||
@Override | ||
public SortedNumericDocValues getSortedNumericDocValues(String field) throws IOException { | ||
return in.getSortedNumericDocValues(field); | ||
} | ||
|
||
@Override | ||
public SortedSetDocValues getSortedSetDocValues(String field) throws IOException { | ||
return in.getSortedSetDocValues(field); | ||
} | ||
} | ||
|
||
/** | ||
* Implementation of {@link QueryTimeout} with a Runnable task. | ||
*/ | ||
private static class Cancellable implements QueryTimeout { | ||
|
||
private Supplier<Runnable> cancellable; | ||
|
||
public void setCancellable(Supplier<Runnable> cancellable) { | ||
this.cancellable = cancellable; | ||
} | ||
|
||
@Override | ||
public boolean shouldExit() { | ||
assert cancellable != null : "checkCancelled must be set immediately after the construction of CancellableIndexReader"; | ||
if (cancellable.get() == null) { | ||
return false; | ||
} | ||
cancellable.get().run(); | ||
return false; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This implementation feels a bit awkward, I'd rather like to fork ExitableDirectoryReader entirely to not inherit from its QueryTimeout abstraction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried that in the 1st approach but this means we have to copy the whole There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mind copying it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to copy |
||
|
||
@Override | ||
public boolean isTimeoutEnabled() { | ||
assert cancellable != null : "checkCancelled must be set immediately after the construction of CancellableIndexReader"; | ||
return cancellable.get() != null; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -261,6 +261,7 @@ private void verifyAvgOfDoubles(double[] values, double expected, double delta) | |
); | ||
} | ||
|
||
@AwaitsFix(bugUrl = "Replace with integration test") | ||
public void testSingleValuedFieldPartiallyUnmapped() throws IOException { | ||
Directory directory = newDirectory(); | ||
RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory); | ||
|
@@ -537,7 +538,7 @@ public void testOrderByEmptyAggregation() throws IOException { | |
indexWriter.close(); | ||
|
||
IndexReader indexReader = DirectoryReader.open(directory); | ||
IndexSearcher indexSearcher = newSearcher(indexReader, true, true); | ||
IndexSearcher indexSearcher = newIndexSearcher(indexReader); | ||
|
||
TermsAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType); | ||
aggregator.preCollection(); | ||
|
@@ -587,7 +588,7 @@ private void testCase(AvgAggregationBuilder aggregationBuilder, Query query, | |
indexWriter.close(); | ||
|
||
IndexReader indexReader = DirectoryReader.open(directory); | ||
IndexSearcher indexSearcher = newSearcher(indexReader, true, true); | ||
IndexSearcher indexSearcher = newIndexSearcher(indexReader); | ||
|
||
AvgAggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType); | ||
aggregator.preCollection(); | ||
|
@@ -611,14 +612,8 @@ public void testCacheAggregation() throws IOException { | |
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, will do. |
||
indexWriter.close(); | ||
|
||
Directory unmappedDirectory = newDirectory(); | ||
RandomIndexWriter unmappedIndexWriter = new RandomIndexWriter(random(), unmappedDirectory); | ||
unmappedIndexWriter.close(); | ||
|
||
IndexReader indexReader = DirectoryReader.open(directory); | ||
IndexReader unamappedIndexReader = DirectoryReader.open(unmappedDirectory); | ||
MultiReader multiReader = new MultiReader(indexReader, unamappedIndexReader); | ||
IndexSearcher indexSearcher = newSearcher(multiReader, true, true); | ||
IndexSearcher indexSearcher = newIndexSearcher(indexReader); | ||
|
||
MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.INTEGER); | ||
fieldType.setName("value"); | ||
|
@@ -639,9 +634,8 @@ public void testCacheAggregation() throws IOException { | |
// Test that an aggregation not using a script does get cached | ||
assertTrue(aggregator.context().getQueryShardContext().isCacheable()); | ||
|
||
multiReader.close(); | ||
indexReader.close(); | ||
directory.close(); | ||
unmappedDirectory.close(); | ||
} | ||
|
||
/** | ||
|
@@ -657,14 +651,8 @@ public void testScriptCaching() throws IOException { | |
} | ||
indexWriter.close(); | ||
|
||
Directory unmappedDirectory = newDirectory(); | ||
RandomIndexWriter unmappedIndexWriter = new RandomIndexWriter(random(), unmappedDirectory); | ||
unmappedIndexWriter.close(); | ||
|
||
IndexReader indexReader = DirectoryReader.open(directory); | ||
IndexReader unamappedIndexReader = DirectoryReader.open(unmappedDirectory); | ||
MultiReader multiReader = new MultiReader(indexReader, unamappedIndexReader); | ||
IndexSearcher indexSearcher = newSearcher(multiReader, true, true); | ||
IndexSearcher indexSearcher = newIndexSearcher(indexReader); | ||
|
||
MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.INTEGER); | ||
fieldType.setName("value"); | ||
|
@@ -705,8 +693,7 @@ public void testScriptCaching() throws IOException { | |
// Test that an aggregation using a nondeterministic script does not get cached | ||
assertFalse(aggregator.context().getQueryShardContext().isCacheable()); | ||
|
||
multiReader.close(); | ||
indexReader.close(); | ||
directory.close(); | ||
unmappedDirectory.close(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this leniency looks dangerous?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I simply propagate the exception? or any other suggestion?