diwu-sf commented on code in PR #16923:
URL: https://github.com/apache/lucene/pull/16923#discussion_r4451980394
##########
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 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: after a commit during which a reader was opened, readers
opened afterwards have a version one higher than the commit on disk. With
frequent refreshes and slow disk syncs, that's most commits. Once the writer is
closed, such a reader says it's out of date even though its data is correct; a
new reader opened from the directory has the same data. There's a test and a
code comment for this, and the PR description explains it.
Also, a reader opened during a commit was still reported as current once the
writer was closed, because `isCurrent()` then only compares versions with the
latest commit on disk. It now also checks that the reader points at that
commit, and there's a test for it.
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]
