Skip to content

Commit

Permalink
Use separate searchers for "search visibility" vs "move indexing buff…
Browse files Browse the repository at this point in the history
…er to disk (#26972)

Today, when ES detects it's using too much heap vs the configured indexing
buffer (default 10% of JVM heap) it opens a new searcher to force Lucene to move
the bytes to disk, clear version map, etc.

But this has the unexpected side effect of making newly indexed/deleted
documents visible to future searches, which is not nice for users who are trying
to prevent that, e.g. #3593.

This is also an indirect spinoff from #26802 where we potentially pay a big
price on rebuilding caches etc. when updates / realtime-get is used. We are
refreshing the internal reader for realtime gets which causes for instance
global ords to be rebuild. I think we can gain quite a bit if we'd use a reader
that is only used for GETs and not for searches etc. that way we can also solve
problems of searchers being refreshed unexpectedly aside of replica recovery /
relocation.

Closes #15768
Closes #26912
  • Loading branch information
s1monw authored Oct 12, 2017
1 parent e1679bf commit 21eb9bd
Show file tree
Hide file tree
Showing 8 changed files with 220 additions and 94 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -305,9 +305,9 @@ private void buildFieldStatistics(XContentBuilder builder, Terms curTerms) throw
long sumDocFreq = curTerms.getSumDocFreq();
int docCount = curTerms.getDocCount();
long sumTotalTermFrequencies = curTerms.getSumTotalTermFreq();
if (docCount > 0) {
assert ((sumDocFreq > 0)) : "docCount >= 0 but sumDocFreq ain't!";
assert ((sumTotalTermFrequencies > 0)) : "docCount >= 0 but sumTotalTermFrequencies ain't!";
if (docCount >= 0) {
assert ((sumDocFreq >= 0)) : "docCount >= 0 but sumDocFreq ain't!";
assert ((sumTotalTermFrequencies >= 0)) : "docCount >= 0 but sumTotalTermFrequencies ain't!";
builder.startObject(FieldStrings.FIELD_STATISTICS);
builder.field(FieldStrings.SUM_DOC_FREQ, sumDocFreq);
builder.field(FieldStrings.DOC_COUNT, docCount);
Expand Down
36 changes: 29 additions & 7 deletions core/src/main/java/org/elasticsearch/index/engine/Engine.java
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.function.Function;
import java.util.function.BiFunction;

public abstract class Engine implements Closeable {

Expand Down Expand Up @@ -465,8 +465,9 @@ public enum SyncedFlushResult {
PENDING_OPERATIONS
}

protected final GetResult getFromSearcher(Get get, Function<String, Searcher> searcherFactory) throws EngineException {
final Searcher searcher = searcherFactory.apply("get");
protected final GetResult getFromSearcher(Get get, BiFunction<String, SearcherScope, Searcher> searcherFactory,
SearcherScope scope) throws EngineException {
final Searcher searcher = searcherFactory.apply("get", scope);
final DocIdAndVersion docIdAndVersion;
try {
docIdAndVersion = VersionsAndSeqNoResolver.loadDocIdAndVersion(searcher.reader(), get.uid());
Expand Down Expand Up @@ -494,23 +495,40 @@ protected final GetResult getFromSearcher(Get get, Function<String, Searcher> se
}
}

public abstract GetResult get(Get get, Function<String, Searcher> searcherFactory) throws EngineException;
public abstract GetResult get(Get get, BiFunction<String, SearcherScope, Searcher> searcherFactory) throws EngineException;


/**
* Returns a new searcher instance. The consumer of this
* API is responsible for releasing the returned searcher in a
* safe manner, preferably in a try/finally block.
*
* @param source the source API or routing that triggers this searcher acquire
*
* @see Searcher#close()
*/
public final Searcher acquireSearcher(String source) throws EngineException {
return acquireSearcher(source, SearcherScope.EXTERNAL);
}

/**
* Returns a new searcher instance. The consumer of this
* API is responsible for releasing the returned searcher in a
* safe manner, preferably in a try/finally block.
*
* @param source the source API or routing that triggers this searcher acquire
* @param scope the scope of this searcher ie. if the searcher will be used for get or search purposes
*
* @see Searcher#close()
*/
public final Searcher acquireSearcher(String source, SearcherScope scope) throws EngineException {
boolean success = false;
/* Acquire order here is store -> manager since we need
* to make sure that the store is not closed before
* the searcher is acquired. */
store.incRef();
try {
final SearcherManager manager = getSearcherManager(); // can never be null
final SearcherManager manager = getSearcherManager(source, scope); // can never be null
/* This might throw NPE but that's fine we will run ensureOpen()
* in the catch block and throw the right exception */
final IndexSearcher searcher = manager.acquire();
Expand All @@ -536,6 +554,10 @@ public final Searcher acquireSearcher(String source) throws EngineException {
}
}

public enum SearcherScope {
EXTERNAL, INTERNAL
}

/** returns the translog for this engine */
public abstract Translog getTranslog();

Expand Down Expand Up @@ -768,7 +790,7 @@ public final boolean refreshNeeded() {
the store is closed so we need to make sure we increment it here
*/
try {
return getSearcherManager().isSearcherCurrent() == false;
return getSearcherManager("refresh_needed", SearcherScope.EXTERNAL).isSearcherCurrent() == false;
} catch (IOException e) {
logger.error("failed to access searcher manager", e);
failEngine("failed to access searcher manager", e);
Expand Down Expand Up @@ -1306,7 +1328,7 @@ public void release() {
}
}

protected abstract SearcherManager getSearcherManager();
protected abstract SearcherManager getSearcherManager(String source, SearcherScope scope);

/**
* Method to close the engine while the write lock is held.
Expand Down
Loading

0 comments on commit 21eb9bd

Please sign in to comment.