ChrisHegarty commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4348865594
##########
lucene/core/src/test/org/apache/lucene/codecs/lucene99/TestVectorsMergeReadAdvice.java:
##########
@@ -199,10 +204,24 @@ public void testAMergeReadsThroughTheReaderItOpened()
throws Exception {
}
}
- /** Searches that do not read the vectors at random leave nothing for a
merge to undo. */
- public void testNoMappingWhenSearchesDoNotReadAtRandom() throws Exception {
- Opens opens = new Opens();
- try (Directory dir = new RecordingDirectory(newDirectory(), opens)) {
+ /** No new input when searches already read the vectors sequentially. */
+ public void testNoMappingWhenSearchesReadInOrder() throws Exception {
+ assertNoMapping(
+ dir -> new RecordingDirectory(dir, new Opens()),
+ IOContext.DEFAULT.withHints(DataAccessHint.SEQUENTIAL));
+ }
+
+ /** No new input when every context reads the same. */
+ public void testNoMappingWhenTheDirectoryReadsTheSame() throws Exception {
+ assertNoMapping(
+ dir -> new RecordingDirectory(dir, new Opens(), false),
+ IOContext.DEFAULT.withHints(DataAccessHint.RANDOM,
NoReuseHint.INSTANCE));
+ }
+
+ private static void assertNoMapping(
+ java.util.function.Function<Directory, RecordingDirectory> wrap,
IOContext searchContext)
Review Comment:
nit: let's import `java.util.function.Function`
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/compressing/Lucene90CompressingStoredFieldsReader.java:
##########
@@ -274,14 +271,16 @@ private void ensureOpen() throws AlreadyClosedException {
@Override
public void close() throws IOException {
if (!closed) {
- IOUtils.close(indexReader, fieldsStream, mergeFieldsStreamToClose());
+ IOUtils.close(indexReader, fieldsStream,
sequentialFieldsStreamToClose());
closed = true;
}
}
- /** The mapping a merge opened, read under the lock that guards it, closed
outside it. */
- private synchronized IndexInput mergeFieldsStreamToClose() {
- return original == this && mergeFieldsStream != fieldsStream ?
mergeFieldsStream : null;
+ /** Returns the sequential input to close, if it is not {@code
fieldsStream}. */
+ private synchronized IndexInput sequentialFieldsStreamToClose() {
+ return original == this && sequentialFieldsStream != fieldsStream
+ ? sequentialFieldsStream
+ : null;
Review Comment:
`close()` takes `sequentialFieldsStreamToClose()` and only sets `closed =
true` after `IOUtils.close(...)` returns. `sequentialFieldsStream()` checks
`ensureOpen()` against that same volatile flag. A concurrent
`getMergeInstance()` can therefore pass `ensureOpen()` after `close()` has
already collected a null stream. It then opens a new input that is never closed.
You avoided this in `Lucene99FlatVectorsReader` with a closed flag set
inside the lock (lines 224-226, 395). Suggest the same here, with closed
checked and set under the monitor.
--
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]
