ChrisHegarty commented on code in PR #16749:
URL: https://github.com/apache/lucene/pull/16749#discussion_r4143810584
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99FlatVectorsReader.java:
##########
@@ -185,9 +210,38 @@ public void checkIntegrity(MergePolicy.OneMerge merge)
throws IOException {
@Override
public FlatVectorsReader getMergeInstance() throws IOException {
- // Update the read advice since vectors are guaranteed to be accessed
sequentially for merge
-
vectorData.updateIOContext(dataContext.withHints(DataAccessHint.SEQUENTIAL));
- return this;
+ return new Lucene99FlatVectorsReader(this,
original.mergeVectorData().clone());
+ }
+
+ /**
+ * The vectors as a merge reads them, front to back and once. Advice belongs
to a mapping, so a
+ * merge maps the file again. Mapped on the first merge, released by {@link
#finishMerge()}.
+ */
+ private synchronized IndexInput mergeVectorData() throws IOException {
+ assert original == this;
+ mergeInstances++;
Review Comment:
`mergeInstances++` in `mergeVectorData()` runs before the mapping is
confirmed open, and unconditionally regardless of whether `getMergeInstance()`
ever successfully returns a reader for it. I see two related gaps becuase of
this:
1. If `directory.openInput(...)` throws anything other than
`FileNotFoundException`/`NoSuchFileException`, the counter is bumped but
`mergeVectorData()` never returns, so `getMergeInstance()` never produces a
reader, and nothing will ever call `finishMerge()` to decrement it back.
2. Even when the open succeeds, `original.mergeVectorData().clone()` could
still throw before `getMergeInstance()` returns, leaving the same kind of
unmatched increment.
Both leave `mergeInstances` permanently inflated relative to live instances,
so `releaseMergeVectorData()` never brings the shared mapping's refcount back
to 0. It ends up staying open until the segment reader's own` close()` runs,
rather than being released promptly when the last real merge instance finishes.
Maybe we should move the increment to just after the mapping is confirmed
open (still inside `mergeVectorData()`'s synchronized block, so it stays atomic
with the open), and wrapping the clone()/constructor call in
`getMergeInstance()` with a catch that calls `releaseMergeVectorData()` to undo
the count on failure.
--
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]
