msokolov commented on a change in pull request #1623:
URL: https://github.com/apache/lucene-solr/pull/1623#discussion_r471017030
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -607,6 +633,57 @@ DirectoryReader getReader(boolean applyAllDeletes, boolean
writeAllDeletes) thro
}
}
}
+ if (onCommitMerges != null) { // only relevant if we do merge on
getReader
+ boolean replaceReaderSuccess = false;
+ try {
+ mergeScheduler.merge(mergeSource, MergeTrigger.GET_READER);
+ onCommitMerges.await(maxCommitMergeWaitMillis,
TimeUnit.MILLISECONDS);
Review comment:
There's no need to check whether we timed out here, since we effectively
abort all of the point-in-time merges we created by setting
`includeMergeReader` to `false` below, right? I wonder if it would be cleaner
to eliminate this AtomicBoolean that is shared with this and the merge threads,
and instead using the existing merge abort technique that we have? OTOH IDK how
that other mechanism works - is it aborting *all* outstanding merges?
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -607,6 +633,57 @@ DirectoryReader getReader(boolean applyAllDeletes, boolean
writeAllDeletes) thro
}
}
}
+ if (onCommitMerges != null) { // only relevant if we do merge on
getReader
+ boolean replaceReaderSuccess = false;
+ try {
+ mergeScheduler.merge(mergeSource, MergeTrigger.GET_READER);
+ onCommitMerges.await(maxCommitMergeWaitMillis,
TimeUnit.MILLISECONDS);
+ assert openingSegmentInfos != null;
+ synchronized (this) {
+ includeMergeReader.set(false);
+ boolean openNewReader = mergedReaders.isEmpty() == false;
+ if (openNewReader) {
+ StandardDirectoryReader mergedReader =
StandardDirectoryReader.open(this,
+ sci -> {
+ // as soon as we remove the reader and return it the
StandardDirectoryReader#open
+ // will take care of closing it. We only need to handle
the readers that remain in the
+ // mergedReaders map and close them.
+ SegmentReader remove = mergedReaders.remove(sci.info.name);
+ if (remove == null) {
+ remove = openedReadOnlyClones.remove(sci.info.name);
+ assert remove != null;
+ // each of the readers we reuse from the previous reader
needs to be refInced
+ // since we reuse them but don't have an implicit refInc
in the SDR:open call
+ remove.incRef();
+ }
+ return remove;
+ }, openingSegmentInfos, applyAllDeletes, writeAllDeletes);
+ try {
+ r.close(); // close and swap in the new reader... close is
cool here since we didn't leak this reader yet
+ } finally {
+ r = mergedReader;
+ }
+ }
+ }
+ replaceReaderSuccess = true;
+ } finally {
+ synchronized (this) {
Review comment:
and this one, closeMergedReaders?
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -607,6 +633,57 @@ DirectoryReader getReader(boolean applyAllDeletes, boolean
writeAllDeletes) thro
}
}
}
+ if (onCommitMerges != null) { // only relevant if we do merge on
getReader
+ boolean replaceReaderSuccess = false;
+ try {
+ mergeScheduler.merge(mergeSource, MergeTrigger.GET_READER);
+ onCommitMerges.await(maxCommitMergeWaitMillis,
TimeUnit.MILLISECONDS);
+ assert openingSegmentInfos != null;
+ synchronized (this) {
Review comment:
This method is getting pretty big, and it might help readability if we
named these synchronized blocks as functions. This one could be
replaceReader()? OTOH maybe it requires too many parameters - it's hard to tell
in code review
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
