Skip to content

Commit f072946

Browse files
s1monwkcm
s1monw
authored and
kcm
committed
Fold EngineSearcher into Engine.Searcher (#34082)
EngineSearcher can be easily folded into Engine.Searcher which removes a level of inheritance that is necessary for most of it's subclasses. This change folds it into Engine.Searcher and removes the dependency on ReferenceManager.
1 parent 1214f0f commit f072946

File tree

9 files changed

+60
-110
lines changed

9 files changed

+60
-110
lines changed

‎server/src/main/java/org/elasticsearch/index/engine/Engine.java‎

Lines changed: 44 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import org.apache.lucene.store.IOContext;
4444
import org.apache.lucene.util.Accountable;
4545
import org.apache.lucene.util.Accountables;
46+
import org.apache.lucene.util.IOUtils;
4647
import org.apache.lucene.util.SetOnce;
4748
import org.elasticsearch.ExceptionsHelper;
4849
import org.elasticsearch.action.index.IndexRequest;
@@ -663,7 +664,15 @@ public Searcher acquireSearcher(String source, SearcherScope scope) throws Engin
663664
}
664665
Releasable releasable = store::decRef;
665666
try {
666-
EngineSearcher engineSearcher = new EngineSearcher(source, getReferenceManager(scope), store, logger);
667+
ReferenceManager<IndexSearcher> referenceManager = getReferenceManager(scope);
668+
Searcher engineSearcher = new Searcher(source, referenceManager.acquire(),
669+
s -> {
670+
try {
671+
referenceManager.release(s);
672+
} finally {
673+
store.decRef();
674+
}
675+
}, logger);
667676
releasable = null; // success - hand over the reference to the engine searcher
668677
return engineSearcher;
669678
} catch (AlreadyClosedException ex) {
@@ -1167,40 +1176,67 @@ default void onFailedEngine(String reason, @Nullable Exception e) {
11671176
}
11681177

11691178
public static class Searcher implements Releasable {
1170-
11711179
private final String source;
11721180
private final IndexSearcher searcher;
1181+
private final AtomicBoolean released = new AtomicBoolean(false);
1182+
private final Logger logger;
1183+
private final IOUtils.IOConsumer<IndexSearcher> onClose;
1184+
1185+
public Searcher(String source, IndexSearcher searcher, Logger logger) {
1186+
this(source, searcher, s -> s.getIndexReader().close(), logger);
1187+
}
11731188

1174-
public Searcher(String source, IndexSearcher searcher) {
1189+
public Searcher(String source, IndexSearcher searcher, IOUtils.IOConsumer<IndexSearcher> onClose, Logger logger) {
11751190
this.source = source;
11761191
this.searcher = searcher;
1192+
this.onClose = onClose;
1193+
this.logger = logger;
11771194
}
11781195

11791196
/**
11801197
* The source that caused this searcher to be acquired.
11811198
*/
1182-
public String source() {
1199+
public final String source() {
11831200
return source;
11841201
}
11851202

1186-
public IndexReader reader() {
1203+
public final IndexReader reader() {
11871204
return searcher.getIndexReader();
11881205
}
11891206

1190-
public DirectoryReader getDirectoryReader() {
1207+
public final DirectoryReader getDirectoryReader() {
11911208
if (reader() instanceof DirectoryReader) {
11921209
return (DirectoryReader) reader();
11931210
}
11941211
throw new IllegalStateException("Can't use " + reader().getClass() + " as a directory reader");
11951212
}
11961213

1197-
public IndexSearcher searcher() {
1214+
public final IndexSearcher searcher() {
11981215
return searcher;
11991216
}
12001217

12011218
@Override
12021219
public void close() {
1203-
// Nothing to close here
1220+
if (released.compareAndSet(false, true) == false) {
1221+
/* In general, searchers should never be released twice or this would break reference counting. There is one rare case
1222+
* when it might happen though: when the request and the Reaper thread would both try to release it in a very short amount
1223+
* of time, this is why we only log a warning instead of throwing an exception.
1224+
*/
1225+
logger.warn("Searcher was released twice", new IllegalStateException("Double release"));
1226+
return;
1227+
}
1228+
try {
1229+
onClose.accept(searcher());
1230+
} catch (IOException e) {
1231+
throw new IllegalStateException("Cannot close", e);
1232+
} catch (AlreadyClosedException e) {
1233+
// This means there's a bug somewhere: don't suppress it
1234+
throw new AssertionError(e);
1235+
}
1236+
}
1237+
1238+
public final Logger getLogger() {
1239+
return logger;
12041240
}
12051241
}
12061242

‎server/src/main/java/org/elasticsearch/index/engine/EngineSearcher.java‎

Lines changed: 0 additions & 68 deletions
This file was deleted.

‎server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -606,7 +606,7 @@ public GetResult get(Get get, BiFunction<String, SearcherScope, Searcher> search
606606
// in the case of a already pruned translog generation we might get null here - yet very unlikely
607607
TranslogLeafReader reader = new TranslogLeafReader((Translog.Index) operation, engineConfig
608608
.getIndexSettings().getIndexVersionCreated());
609-
return new GetResult(new Searcher("realtime_get", new IndexSearcher(reader)),
609+
return new GetResult(new Searcher("realtime_get", new IndexSearcher(reader), logger),
610610
new VersionsAndSeqNoResolver.DocIdAndVersion(0, ((Translog.Index) operation).version(), reader, 0));
611611
}
612612
} catch (IOException e) {
@@ -2085,7 +2085,7 @@ public IndexSearcher newSearcher(IndexReader reader, IndexReader previousReader)
20852085
if (warmer != null) {
20862086
try {
20872087
assert searcher.getIndexReader() instanceof ElasticsearchDirectoryReader : "this class needs an ElasticsearchDirectoryReader but got: " + searcher.getIndexReader().getClass();
2088-
warmer.warm(new Searcher("top_reader_warming", searcher));
2088+
warmer.warm(new Searcher("top_reader_warming", searcher, s -> {}, logger));
20892089
} catch (Exception e) {
20902090
if (isEngineClosed.get() == false) {
20912091
logger.warn("failed to prepare/warm", e);

‎server/src/main/java/org/elasticsearch/index/shard/IndexSearcherWrapper.java‎

Lines changed: 5 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@
2424
import org.apache.lucene.index.IndexReader;
2525
import org.apache.lucene.index.LeafReader;
2626
import org.apache.lucene.search.IndexSearcher;
27-
import org.elasticsearch.ElasticsearchException;
2827
import org.elasticsearch.common.lucene.index.ElasticsearchDirectoryReader;
28+
import org.elasticsearch.core.internal.io.IOUtils;
2929
import org.elasticsearch.index.engine.Engine;
3030

3131
import java.io.IOException;
@@ -97,21 +97,10 @@ public final Engine.Searcher wrap(Engine.Searcher engineSearcher) throws IOExcep
9797
if (reader == nonClosingReaderWrapper && indexSearcher == innerIndexSearcher) {
9898
return engineSearcher;
9999
} else {
100-
return new Engine.Searcher(engineSearcher.source(), indexSearcher) {
101-
@Override
102-
public void close() throws ElasticsearchException {
103-
try {
104-
reader().close();
105-
// we close the reader to make sure wrappers can release resources if needed....
106-
// our NonClosingReaderWrapper makes sure that our reader is not closed
107-
} catch (IOException e) {
108-
throw new ElasticsearchException("failed to close reader", e);
109-
} finally {
110-
engineSearcher.close();
111-
}
112-
113-
}
114-
};
100+
// we close the reader to make sure wrappers can release resources if needed....
101+
// our NonClosingReaderWrapper makes sure that our reader is not closed
102+
return new Engine.Searcher(engineSearcher.source(), indexSearcher, s -> IOUtils.close(s.getIndexReader(), engineSearcher),
103+
engineSearcher.getLogger());
115104
}
116105
}
117106

‎server/src/test/java/org/elasticsearch/index/shard/IndexSearcherWrapperTests.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ public IndexSearcher wrap(IndexSearcher searcher) throws EngineException {
7373
final int sourceRefCount = open.getRefCount();
7474
final AtomicInteger count = new AtomicInteger();
7575
final AtomicInteger outerCount = new AtomicInteger();
76-
try (Engine.Searcher engineSearcher = new Engine.Searcher("foo", searcher)) {
76+
try (Engine.Searcher engineSearcher = new Engine.Searcher("foo", searcher, s -> {}, logger)) {
7777
final Engine.Searcher wrap = wrapper.wrap(engineSearcher);
7878
assertEquals(1, wrap.reader().getRefCount());
7979
ElasticsearchDirectoryReader.addReaderCloseListener(wrap.getDirectoryReader(), key -> {
@@ -121,7 +121,7 @@ public IndexSearcher wrap(IndexSearcher searcher) throws EngineException {
121121
}
122122
};
123123
final ConcurrentHashMap<Object, TopDocs> cache = new ConcurrentHashMap<>();
124-
try (Engine.Searcher engineSearcher = new Engine.Searcher("foo", searcher)) {
124+
try (Engine.Searcher engineSearcher = new Engine.Searcher("foo", searcher, s -> {}, logger)) {
125125
try (Engine.Searcher wrap = wrapper.wrap(engineSearcher)) {
126126
ElasticsearchDirectoryReader.addReaderCloseListener(wrap.getDirectoryReader(), key -> {
127127
cache.remove(key);
@@ -151,7 +151,7 @@ public void testNoWrap() throws IOException {
151151
assertEquals(1, searcher.search(new TermQuery(new Term("field", "doc")), 1).totalHits.value);
152152
searcher.setSimilarity(iwc.getSimilarity());
153153
IndexSearcherWrapper wrapper = new IndexSearcherWrapper();
154-
try (Engine.Searcher engineSearcher = new Engine.Searcher("foo", searcher)) {
154+
try (Engine.Searcher engineSearcher = new Engine.Searcher("foo", searcher, logger)) {
155155
final Engine.Searcher wrap = wrapper.wrap(engineSearcher);
156156
assertSame(wrap, engineSearcher);
157157
}

‎server/src/test/java/org/elasticsearch/search/DefaultSearchContextTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ public void testPreProcess() throws Exception {
110110
try (Directory dir = newDirectory();
111111
RandomIndexWriter w = new RandomIndexWriter(random(), dir);
112112
IndexReader reader = w.getReader();
113-
Engine.Searcher searcher = new Engine.Searcher("test", new IndexSearcher(reader))) {
113+
Engine.Searcher searcher = new Engine.Searcher("test", new IndexSearcher(reader), logger)) {
114114

115115
DefaultSearchContext context1 = new DefaultSearchContext(1L, shardSearchRequest, null, searcher, null, indexService,
116116
indexShard, bigArrays, null, timeout, null, null, Version.CURRENT);

‎server/src/test/java/org/elasticsearch/search/profile/query/QueryProfilerTests.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ public static void setup() throws IOException {
8282
}
8383
reader = w.getReader();
8484
w.close();
85-
Engine.Searcher engineSearcher = new Engine.Searcher("test", new IndexSearcher(reader));
85+
Engine.Searcher engineSearcher = new Engine.Searcher("test", new IndexSearcher(reader), null);
8686
searcher = new ContextIndexSearcher(engineSearcher, IndexSearcher.getDefaultQueryCache(), MAYBE_CACHE_POLICY);
8787
}
8888

@@ -363,7 +363,7 @@ public void testUseIndexStats() throws IOException {
363363

364364
public void testApproximations() throws IOException {
365365
QueryProfiler profiler = new QueryProfiler();
366-
Engine.Searcher engineSearcher = new Engine.Searcher("test", new IndexSearcher(reader));
366+
Engine.Searcher engineSearcher = new Engine.Searcher("test", new IndexSearcher(reader), logger);
367367
// disable query caching since we want to test approximations, which won't
368368
// be exposed on a cached entry
369369
ContextIndexSearcher searcher = new ContextIndexSearcher(engineSearcher, null, MAYBE_CACHE_POLICY);

‎test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ protected <A extends Aggregator> A createAggregator(Query query,
240240
}
241241

242242
protected SearchContext createSearchContext(IndexSearcher indexSearcher, IndexSettings indexSettings) {
243-
Engine.Searcher searcher = new Engine.Searcher("aggregator_test", indexSearcher);
243+
Engine.Searcher searcher = new Engine.Searcher("aggregator_test", indexSearcher, logger);
244244
QueryCache queryCache = new DisabledQueryCache(indexSettings);
245245
QueryCachingPolicy queryCachingPolicy = new QueryCachingPolicy() {
246246
@Override

‎test/framework/src/main/java/org/elasticsearch/test/engine/AssertingSearcher.java‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,8 @@ class AssertingSearcher extends Engine.Searcher {
3838
private final Logger logger;
3939
private final AtomicBoolean closed = new AtomicBoolean(false);
4040

41-
AssertingSearcher(IndexSearcher indexSearcher, final Engine.Searcher wrappedSearcher,
42-
ShardId shardId,
43-
Logger logger) {
44-
super(wrappedSearcher.source(), indexSearcher);
41+
AssertingSearcher(IndexSearcher indexSearcher, final Engine.Searcher wrappedSearcher, ShardId shardId, Logger logger) {
42+
super(wrappedSearcher.source(), indexSearcher, s -> {throw new AssertionError();}, logger);
4543
// we only use the given index searcher here instead of the IS of the wrapped searcher. the IS might be a wrapped searcher
4644
// with a wrapped reader.
4745
this.wrappedSearcher = wrappedSearcher;
@@ -52,11 +50,6 @@ class AssertingSearcher extends Engine.Searcher {
5250
"IndexReader#getRefCount() was [" + initialRefCount + "] expected a value > [0] - reader is already closed";
5351
}
5452

55-
@Override
56-
public String source() {
57-
return wrappedSearcher.source();
58-
}
59-
6053
@Override
6154
public void close() {
6255
synchronized (lock) {

0 commit comments

Comments
 (0)