Don't apply the plugin's reader wrapper in can_match phase (#47816)

This change modifies the local execution of the `can_match` phase to **not** apply
the plugin's reader wrapper (if it is configured) when acquiring the searcher.
We must ensure that the phase runs quickly and since we don't know the cost
of applying the wrapper it is preferable to avoid it entirely. The can_match
phase can aford false positives so it is also safe for the builtin plugins
that use this functionality.

Closes #46817
This commit is contained in:
Jim Ferenczi 2019-10-14 13:06:36 +02:00 committed by jimczi
parent 9ee7b3743e
commit ef02a736ca
3 changed files with 68 additions and 5 deletions

View File

@ -1216,6 +1216,15 @@ public class IndexShard extends AbstractIndexShardComponent implements IndicesCl
getEngine().failEngine(reason, e); getEngine().failEngine(reason, e);
} }
/**
* Acquire the searcher without applying the additional reader wrapper.
*/
public Engine.Searcher acquireSearcherNoWrap(String source) {
readAllowed();
markSearcherAccessed();
return getEngine().acquireSearcher(source, Engine.SearcherScope.EXTERNAL);
}
public Engine.Searcher acquireSearcher(String source) { public Engine.Searcher acquireSearcher(String source) {
return acquireSearcher(source, Engine.SearcherScope.EXTERNAL); return acquireSearcher(source, Engine.SearcherScope.EXTERNAL);
} }

View File

@ -1012,10 +1012,16 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv
*/ */
public boolean canMatch(ShardSearchRequest request) throws IOException { public boolean canMatch(ShardSearchRequest request) throws IOException {
assert request.searchType() == SearchType.QUERY_THEN_FETCH : "unexpected search type: " + request.searchType(); assert request.searchType() == SearchType.QUERY_THEN_FETCH : "unexpected search type: " + request.searchType();
try (DefaultSearchContext context = createSearchContext(request, defaultSearchTimeout, false, "can_match")) { IndexService indexService = indicesService.indexServiceSafe(request.shardId().getIndex());
SearchSourceBuilder source = context.request().source(); IndexShard indexShard = indexService.getShard(request.shardId().getId());
if (canRewriteToMatchNone(source)) { // we don't want to use the reader wrapper since it could run costly operations
QueryBuilder queryBuilder = source.query(); // and we can afford false positives.
try (Engine.Searcher searcher = indexShard.acquireSearcherNoWrap("can_match")) {
QueryShardContext context = indexService.newQueryShardContext(request.shardId().id(), searcher,
request::nowInMillis, request.getClusterAlias());
Rewriteable.rewrite(request.getRewriteable(), context, false);
if (canRewriteToMatchNone(request.source())) {
QueryBuilder queryBuilder = request.source().query();
return queryBuilder instanceof MatchNoneQueryBuilder == false; return queryBuilder instanceof MatchNoneQueryBuilder == false;
} }
return true; // null query means match_all return true; // null query means match_all

View File

@ -19,6 +19,9 @@
package org.elasticsearch.search; package org.elasticsearch.search;
import com.carrotsearch.hppc.IntArrayList; import com.carrotsearch.hppc.IntArrayList;
import org.apache.lucene.index.DirectoryReader;
import org.apache.lucene.index.FilterDirectoryReader;
import org.apache.lucene.index.LeafReader;
import org.apache.lucene.search.Query; import org.apache.lucene.search.Query;
import org.apache.lucene.store.AlreadyClosedException; import org.apache.lucene.store.AlreadyClosedException;
import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchException;
@ -76,6 +79,7 @@ import org.elasticsearch.search.internal.SearchContext;
import org.elasticsearch.search.internal.ShardSearchRequest; import org.elasticsearch.search.internal.ShardSearchRequest;
import org.elasticsearch.search.suggest.SuggestBuilder; import org.elasticsearch.search.suggest.SuggestBuilder;
import org.elasticsearch.test.ESSingleNodeTestCase; import org.elasticsearch.test.ESSingleNodeTestCase;
import org.junit.Before;
import java.io.IOException; import java.io.IOException;
import java.util.Collection; import java.util.Collection;
@ -88,6 +92,7 @@ import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutionException;
import java.util.concurrent.Semaphore; import java.util.concurrent.Semaphore;
import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Function; import java.util.function.Function;
import static java.util.Collections.singletonList; import static java.util.Collections.singletonList;
@ -111,7 +116,42 @@ public class SearchServiceTests extends ESSingleNodeTestCase {
@Override @Override
protected Collection<Class<? extends Plugin>> getPlugins() { protected Collection<Class<? extends Plugin>> getPlugins() {
return pluginList(FailOnRewriteQueryPlugin.class, CustomScriptPlugin.class, InternalOrPrivateSettingsPlugin.class); return pluginList(FailOnRewriteQueryPlugin.class, CustomScriptPlugin.class,
ReaderWrapperCountPlugin.class, InternalOrPrivateSettingsPlugin.class);
}
public static class ReaderWrapperCountPlugin extends Plugin {
@Override
public void onIndexModule(IndexModule indexModule) {
indexModule.setReaderWrapper(service -> SearchServiceTests::apply);
}
}
@Before
private void resetCount() {
numWrapInvocations = new AtomicInteger(0);
}
private static AtomicInteger numWrapInvocations = new AtomicInteger(0);
private static DirectoryReader apply(DirectoryReader directoryReader) throws IOException {
numWrapInvocations.incrementAndGet();
return new FilterDirectoryReader(directoryReader,
new FilterDirectoryReader.SubReaderWrapper() {
@Override
public LeafReader wrap(LeafReader reader) {
return reader;
}
}) {
@Override
protected DirectoryReader doWrapDirectoryReader(DirectoryReader in) throws IOException {
return in;
}
@Override
public CacheHelper getReaderCacheHelper() {
return directoryReader.getReaderCacheHelper();
}
};
} }
public static class CustomScriptPlugin extends MockScriptPlugin { public static class CustomScriptPlugin extends MockScriptPlugin {
@ -559,6 +599,7 @@ public class SearchServiceTests extends ESSingleNodeTestCase {
final IndexService indexService = indicesService.indexServiceSafe(resolveIndex("index")); final IndexService indexService = indicesService.indexServiceSafe(resolveIndex("index"));
final IndexShard indexShard = indexService.getShard(0); final IndexShard indexShard = indexService.getShard(0);
SearchRequest searchRequest = new SearchRequest().allowPartialSearchResults(true); SearchRequest searchRequest = new SearchRequest().allowPartialSearchResults(true);
int numWrapReader = numWrapInvocations.get();
assertTrue(service.canMatch(new ShardSearchRequest(OriginalIndices.NONE, searchRequest, indexShard.shardId(), 1, assertTrue(service.canMatch(new ShardSearchRequest(OriginalIndices.NONE, searchRequest, indexShard.shardId(), 1,
new AliasFilter(null, Strings.EMPTY_ARRAY), 1f, -1, null, null))); new AliasFilter(null, Strings.EMPTY_ARRAY), 1f, -1, null, null)));
@ -582,6 +623,13 @@ public class SearchServiceTests extends ESSingleNodeTestCase {
searchRequest.source(new SearchSourceBuilder().query(new MatchNoneQueryBuilder())); searchRequest.source(new SearchSourceBuilder().query(new MatchNoneQueryBuilder()));
assertFalse(service.canMatch(new ShardSearchRequest(OriginalIndices.NONE, searchRequest, indexShard.shardId(), 1, assertFalse(service.canMatch(new ShardSearchRequest(OriginalIndices.NONE, searchRequest, indexShard.shardId(), 1,
new AliasFilter(null, Strings.EMPTY_ARRAY), 1f, -1, null, null))); new AliasFilter(null, Strings.EMPTY_ARRAY), 1f, -1, null, null)));
assertEquals(numWrapReader, numWrapInvocations.get());
// make sure that the wrapper is called when the context is actually created
service.createContext(new ShardSearchRequest(OriginalIndices.NONE, searchRequest,
indexShard.shardId(), 1, new AliasFilter(null, Strings.EMPTY_ARRAY),
1f, -1, null, null)).close();
assertEquals(numWrapReader+1, numWrapInvocations.get());
} }
public void testCanRewriteToMatchNone() { public void testCanRewriteToMatchNone() {