Skip to content

Commit

Permalink
Do not wrap can_match searchers (#64742)
Browse files Browse the repository at this point in the history
The can_match phase does not work with frozen indices that have 
document-level security enabled. Also, we should not wrap can_match
searchers to reduce the load during the can_match phase.
  • Loading branch information
dnhatn committed Nov 8, 2020
1 parent ab0f5c0 commit 0d448db
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ public abstract class Engine implements Closeable {
public static final String HISTORY_UUID_KEY = "history_uuid";
public static final String MIN_RETAINED_SEQNO = "min_retained_seq_no";
public static final String MAX_UNSAFE_AUTO_ID_TIMESTAMP_COMMIT_ID = "max_unsafe_auto_id_timestamp";
public static final String CAN_MATCH_SEARCH_SOURCE = "can_match";


protected final ShardId shardId;
protected final String allocationId;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1270,8 +1270,12 @@ private Engine.Searcher acquireSearcher(String source, Engine.SearcherScope scop
!= null : "DirectoryReader must be an instance or ElasticsearchDirectoryReader";
boolean success = false;
try {
final Engine.Searcher wrappedSearcher = searcherWrapper == null ? searcher : searcherWrapper.wrap(searcher);
assert wrappedSearcher != null;
final Engine.Searcher wrappedSearcher;
if (searcherWrapper != null && Engine.CAN_MATCH_SEARCH_SOURCE.equals(source) == false) {
wrappedSearcher = searcherWrapper.wrap(searcher);
} else {
wrappedSearcher = searcher;
}
success = true;
return wrappedSearcher;
} catch (IOException ex) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1062,7 +1062,7 @@ public AliasFilter buildAliasFilter(ClusterState state, String index, Set<String
*/
public boolean canMatch(ShardSearchRequest request) throws IOException {
assert request.searchType() == SearchType.QUERY_THEN_FETCH : "unexpected search type: " + request.searchType();
try (DefaultSearchContext context = createSearchContext(request, defaultSearchTimeout, false, "can_match")) {
try (DefaultSearchContext context = createSearchContext(request, defaultSearchTimeout, false, Engine.CAN_MATCH_SEARCH_SOURCE)) {
SearchSourceBuilder source = context.request().source();
if (canRewriteToMatchNone(source)) {
QueryBuilder queryBuilder = source.query();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ public Searcher acquireSearcher(String source, SearcherScope scope) throws Engin
case "segments":
case "segments_stats":
case "completion_stats":
case "can_match": // special case for can_match phase - we use the cached point values reader
case CAN_MATCH_SEARCH_SOURCE: // special case for can_match phase - we use the cached point values reader
maybeOpenReader = false;
break;
default:
Expand All @@ -235,7 +235,7 @@ public Searcher acquireSearcher(String source, SearcherScope scope) throws Engin
// we just hand out a searcher on top of an empty reader that we opened for the ReadOnlyEngine in the #open(IndexCommit)
// method. this is the case when we don't have a reader open right now and we get a stats call any other that falls in
// the category that doesn't trigger a reopen
if ("can_match".equals(source)) {
if (CAN_MATCH_SEARCH_SOURCE.equals(source)) {
canMatchReader.incRef();
return new Searcher(source, new IndexSearcher(canMatchReader), canMatchReader::decRef);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ public void testCanMatch() throws IOException {
listener.reset();
try (FrozenEngine frozenEngine = new FrozenEngine(engine.engineConfig)) {
DirectoryReader reader;
try (Engine.Searcher searcher = frozenEngine.acquireSearcher("can_match")) {
try (Engine.Searcher searcher = frozenEngine.acquireSearcher(Engine.CAN_MATCH_SEARCH_SOURCE)) {
assertNotNull(ElasticsearchDirectoryReader.getElasticsearchDirectoryReader(searcher.getDirectoryReader()));
assertEquals(config.getShardId(), ElasticsearchDirectoryReader.getElasticsearchDirectoryReader(searcher
.getDirectoryReader()).shardId());
Expand All @@ -313,7 +313,7 @@ public void testCanMatch() throws IOException {
assertNotNull(ElasticsearchDirectoryReader.getElasticsearchDirectoryReader(searcher.getDirectoryReader()));
}

try (Engine.Searcher searcher = frozenEngine.acquireSearcher("can_match")) {
try (Engine.Searcher searcher = frozenEngine.acquireSearcher(Engine.CAN_MATCH_SEARCH_SOURCE)) {
assertSame(reader, searcher.getDirectoryReader());
assertNotEquals(reader, Matchers.instanceOf(FrozenEngine.LazyDirectoryReader.class));
assertEquals(0, listener.afterRefresh.get());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.index.engine;

import org.apache.lucene.index.DirectoryReader;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.admin.cluster.state.ClusterStateResponse;
import org.elasticsearch.action.admin.indices.stats.IndicesStatsResponse;
Expand All @@ -16,15 +17,18 @@
import org.elasticsearch.cluster.block.ClusterBlockException;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.cluster.routing.RecoverySource;
import org.elasticsearch.common.CheckedFunction;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.IndexModule;
import org.elasticsearch.index.IndexService;
import org.elasticsearch.index.query.MatchAllQueryBuilder;
import org.elasticsearch.index.query.QueryBuilders;
import org.elasticsearch.index.shard.IndexSearcherWrapper;
import org.elasticsearch.index.shard.IndexShard;
import org.elasticsearch.index.shard.IndexShardTestCase;
import org.elasticsearch.indices.IndicesService;
Expand All @@ -40,12 +44,14 @@
import org.elasticsearch.xpack.core.XPackPlugin;
import org.elasticsearch.xpack.core.action.TransportFreezeIndexAction;
import org.hamcrest.Matchers;
import org.junit.Before;

import java.io.IOException;
import java.util.Collection;
import java.util.EnumSet;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.atomic.AtomicReference;

import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
Expand All @@ -60,9 +66,33 @@ public class FrozenIndexTests extends ESSingleNodeTestCase {

@Override
protected Collection<Class<? extends Plugin>> getPlugins() {
return pluginList(XPackPlugin.class);
return pluginList(XPackPlugin.class, SearcherWrapperPlugin.class);
}

static final AtomicReference<CheckedFunction<DirectoryReader, DirectoryReader, IOException>> readerWrapper = new AtomicReference<>();

@Before
public void resetReaderWrapper() throws Exception {
readerWrapper.set(null);
}

public static class SearcherWrapperPlugin extends Plugin {
@Override
public void onIndexModule(IndexModule indexModule) {
super.onIndexModule(indexModule);
if (randomBoolean()) {
indexModule.setSearcherWrapper(indexService -> new IndexSearcherWrapper() {
@Override
protected DirectoryReader wrap(DirectoryReader reader) throws IOException {
final CheckedFunction<DirectoryReader, DirectoryReader, IOException> wrapper = readerWrapper.get();
return wrapper != null ? wrapper.apply(reader) : reader;
}
});
}
}
}


public void testCloseFreezeAndOpen() throws ExecutionException, InterruptedException {
createIndex("index", Settings.builder().put("index.number_of_shards", 2).build());
client().prepareIndex("index", "_doc", "1").setSource("field", "value").setRefreshPolicy(IMMEDIATE).get();
Expand Down Expand Up @@ -241,6 +271,11 @@ public void testFreezePattern() throws ExecutionException, InterruptedException
}

public void testCanMatch() throws ExecutionException, InterruptedException, IOException {
if (randomBoolean()) {
readerWrapper.set(reader -> {
throw new AssertionError("can_match must not wrap the reader");
});
}
createIndex("index");
client().prepareIndex("index", "_doc", "1").setSource("field", "2010-01-05T02:00").setRefreshPolicy(IMMEDIATE).execute()
.actionGet();
Expand Down

0 comments on commit 0d448db

Please sign in to comment.