-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Speed up InternalEngine#resolveDocVersion(...) method #122374
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
Changes from 4 commits
a04181a
d3af272
ce44c20
8072907
b81fa73
247c240
552ad61
1ee2699
9daa2dd
5c4028d
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 |
|---|---|---|
|
|
@@ -70,6 +70,7 @@ | |
| import org.elasticsearch.core.IOUtils; | ||
| import org.elasticsearch.core.Nullable; | ||
| import org.elasticsearch.core.Releasable; | ||
| import org.elasticsearch.core.Releasables; | ||
| import org.elasticsearch.core.SuppressForbidden; | ||
| import org.elasticsearch.core.Tuple; | ||
| import org.elasticsearch.env.Environment; | ||
|
|
@@ -106,6 +107,7 @@ | |
|
|
||
| import java.io.Closeable; | ||
| import java.io.IOException; | ||
| import java.io.UncheckedIOException; | ||
| import java.util.Arrays; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
@@ -1021,21 +1023,13 @@ private VersionValue resolveDocVersion(final Operation op, boolean loadSeqNo) th | |
| if (versionValue == null) { | ||
| assert incrementIndexVersionLookup(); // used for asserting in tests | ||
| final VersionsAndSeqNoResolver.DocIdAndVersion docIdAndVersion; | ||
| try (Searcher searcher = acquireSearcher("load_version", SearcherScope.INTERNAL)) { | ||
| try (var directoryReaderSupplier = acquireDirectoryReaderSupplier(SearcherScope.INTERNAL)) { | ||
| var indexReader = directoryReaderSupplier.getDirectoryReader(); | ||
| if (engineConfig.getIndexSettings().getMode() == IndexMode.TIME_SERIES) { | ||
| assert engineConfig.getLeafSorter() == DataStream.TIMESERIES_LEAF_READERS_SORTER; | ||
| docIdAndVersion = VersionsAndSeqNoResolver.timeSeriesLoadDocIdAndVersion( | ||
| searcher.getIndexReader(), | ||
| op.uid(), | ||
| op.id(), | ||
| loadSeqNo | ||
| ); | ||
| docIdAndVersion = VersionsAndSeqNoResolver.timeSeriesLoadDocIdAndVersion(indexReader, op.uid(), op.id(), loadSeqNo); | ||
| } else { | ||
| docIdAndVersion = VersionsAndSeqNoResolver.timeSeriesLoadDocIdAndVersion( | ||
| searcher.getIndexReader(), | ||
| op.uid(), | ||
| loadSeqNo | ||
| ); | ||
| docIdAndVersion = VersionsAndSeqNoResolver.timeSeriesLoadDocIdAndVersion(indexReader, op.uid(), loadSeqNo); | ||
| } | ||
| } | ||
| if (docIdAndVersion != null) { | ||
|
|
@@ -3470,4 +3464,70 @@ public LiveVersionMap getLiveVersionMap() { | |
| protected long getPreCommitSegmentGeneration() { | ||
| return preCommitSegmentGeneration.get(); | ||
| } | ||
|
|
||
| DirectoryReaderSupplier acquireDirectoryReaderSupplier(SearcherScope scope) throws EngineException { | ||
| assert scope == SearcherScope.INTERNAL : "acquireDirectoryReaderSupplier(...) isn't prepared for external usage"; | ||
|
|
||
| /* Acquire order here is store -> manager since we need | ||
| * to make sure that the store is not closed before | ||
| * the searcher is acquired. */ | ||
| if (store.tryIncRef() == false) { | ||
| throw new AlreadyClosedException(shardId + " store is closed", failedEngine.get()); | ||
| } | ||
| Releasable releasable = store::decRef; | ||
| try { | ||
| ReferenceManager<ElasticsearchDirectoryReader> referenceManager = getReferenceManager(scope); | ||
| ElasticsearchDirectoryReader acquire = referenceManager.acquire(); | ||
| releasable = null; // success - hand over the reference to the engine reader | ||
| return new DirectoryReaderSupplier(acquire) { | ||
|
|
||
| @Override | ||
| void doClose() { | ||
| try { | ||
| referenceManager.release(acquire); | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException("failed to close", e); | ||
| } catch (AlreadyClosedException e) { | ||
| // This means there's a bug somewhere: don't suppress it | ||
| throw new AssertionError(e); | ||
| } finally { | ||
| store.decRef(); | ||
| } | ||
| } | ||
| }; | ||
| } catch (AlreadyClosedException ex) { | ||
| throw ex; | ||
| } catch (Exception ex) { | ||
| maybeFailEngine("acquire_reader", ex); | ||
| ensureOpen(ex); // throw EngineCloseException here if we are already closed | ||
| logger.error("failed to acquire reader", ex); | ||
| throw new EngineException(shardId, "failed to acquire reader", ex); | ||
| } finally { | ||
| Releasables.close(releasable); | ||
| } | ||
| } | ||
|
|
||
| abstract static class DirectoryReaderSupplier implements Releasable { | ||
|
||
| private final DirectoryReader directoryReader; | ||
| private final AtomicBoolean released = new AtomicBoolean(false); | ||
|
|
||
| DirectoryReaderSupplier(DirectoryReader directoryReader) { | ||
| this.directoryReader = directoryReader; | ||
| } | ||
|
|
||
| public DirectoryReader getDirectoryReader() { | ||
| return directoryReader; | ||
| } | ||
|
|
||
| @Override | ||
| public final void close() { | ||
| if (released.compareAndSet(false, true)) { | ||
| doClose(); | ||
| } else { | ||
| assert false : "DirectoryReaderSupplier was released twice"; | ||
| } | ||
| } | ||
|
|
||
| abstract void doClose(); | ||
| } | ||
| } | ||
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.
Ah see @tlrx, we don't need to ever acquire a store because we are guaranteed to have an engine reference here?
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.
I see, so we can just change that into an assertion?
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.
I think so yes, assertion should be enough :)
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.
I think so too
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.
pushed: b81fa73