diwu-sf commented on code in PR #16923:
URL: https://github.com/apache/lucene/pull/16923#discussion_r4451980098
##########
lucene/core/src/java/org/apache/lucene/index/IndexWriter.java:
##########
Review Comment:
Done in 60e2bc2: `nrtIsCurrent` now also logs
`readerPool.anyDocValuesChanges()`.
##########
lucene/core/src/java/org/apache/lucene/index/StandardDirectoryReader.java:
##########
@@ -463,10 +463,12 @@ private DirectoryReader doOpenFromWriter(IndexCommit
commit, ExecutorService exe
return null;
}
- DirectoryReader reader = writer.getReader(applyAllDeletes,
writeAllDeletes);
+ StandardDirectoryReader reader = writer.getReader(applyAllDeletes,
writeAllDeletes);
- // If in fact no changes took place, return null:
- if (reader.getVersion() == segmentInfos.getVersion()) {
+ // If in fact no changes took place, return null. A commit that was
syncing files when this
+ // reader was opened changes the referenced segments_N once it completes,
but not the version:
+ if (reader.getVersion() == segmentInfos.getVersion()
+ && reader.segmentInfos.getLastGeneration() ==
segmentInfos.getLastGeneration()) {
Review Comment:
Thanks, you're right. A reader opened while a commit was still in progress,
and the reader opened right after it, could end up with the same version.
`SearcherLifetimeManager` can't handle that, and the test failure you saw comes
from it.
I couldn't reproduce it with your seed on my machine, so I wrote a test that
pauses the commit at the right moment. It now fails every time on the old code.
Fixed in 60e2bc2 by bumping the version, as you suggested, with one change:
I only bump it if a reader was opened while the commit was in progress. Bumping
on every commit breaks an existing test (`testAddCloseOpen`), which expects a
reader opened right after a commit to have the same version as that commit.
One side effect, only in that rare case: once the writer is closed, a reader
opened after that commit says it's out of date even though it isn't. Reopening
it just gives back the same data, so it costs an extra reopen but nothing
breaks. There's a test and a code comment for this.
I moved the test into a new `TestNRTReaderDuringCommit` and added more
cases: two-step commits (`prepareCommit()` then `commit()`), changes made
during a commit, a commit that fails, your `SearcherLifetimeManager` case,
making sure commits with no changes don't change the version, and a randomized
test. The tests pause commits with latches rather than sleeps. The randomized
test fails every time on the old code and passes with the fix. `./gradlew
:lucene:core:check` and the nightly replication tests pass.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
