ChrisHegarty commented on code in PR #16883:
URL: https://github.com/apache/lucene/pull/16883#discussion_r4358950903
##########
lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java:
##########
Review Comment:
Nit: isEfficientFor mixes two read advice functions if setReadAdvice is
called after an input is opened.
`readAdvice` is captured when the input is created, but the `toReadAdvice`
lambda reads `MMapDirectory.this.readAdvice` each time it runs. After
`setReadAdvice(...`), `isEfficientFor` compares advice stored under the old
function with advice computed under the new one. The field is also
non-volatile, so another thread may not see the new function.
Capturing the function at open time keeps each input (and its clones and
slices) consistent with the function it was opened under:
```suggest
// the function in effect now, so that this input is not affected by a
later setReadAdvice
final var readAdviceFunction = readAdvice;
Function<IOContext, ReadAdvice> toReadAdvice =
c -> readAdviceFunction.apply(name,
c).orElse(Constants.DEFAULT_READADVICE);
```
<details>
<summary>Test that fails without this and passes with it (in
<code>TestMMapDirectory</code>)</summary>
```
/** An input keeps answering from the read advice function it was opened
with. */
public void testIsEfficientForIgnoresLaterReadAdviceChanges() throws
IOException {
assumeTrue("needs madvise to tell random from sequential",
MMapDirectory.supportsMadvise());
var random = new DefaultIOContext(DataAccessHint.RANDOM);
var sequential = new DefaultIOContext(DataAccessHint.SEQUENTIAL);
final int size = 1 << 17;
try (MMapDirectory dir = new
MMapDirectory(createTempDir("testIsEfficientForLater"))) {
try (IndexOutput out = dir.createOutput("test", IOContext.DEFAULT)) {
out.writeBytes(new byte[size], 0, size);
}
dir.setReadAdvice(MMapDirectory.ADVISE_BY_CONTEXT);
try (IndexInput in = dir.openInput("test", random)) {
assertFalse(
"random pages are not efficient for sequential reads",
in.isEfficientFor(sequential));
// changing the function after the open must not change what this
input answers
dir.setReadAdvice((_, _) -> Optional.of(ReadAdvice.NORMAL));
assertFalse("answer changed after setReadAdvice",
in.isEfficientFor(sequential));
assertFalse("a clone answers like its input",
in.clone().isEfficientFor(sequential));
assertFalse(
"a slice answers like its input", in.slice("slice", 0,
size).isEfficientFor(sequential));
}
}
}
```
</details>
--
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]
