Update the IOContext on IndexInput rather than the ReadAdvice #14702
Conversation
github-project-automation
bot
moved this to Open
in OpenSearch Lucene & Core Performance Tracking
github-actions
bot
added
module:core/store
module:core/codecs
module:test-framework
labels
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog-check label to it and you will stop receiving this reminder on future updates to the PR. |
| @@ -124,7 +124,7 @@ public abstract void search( | |||
| * | |||
| * <p>The default implementation returns {@code this} | |||
| */ | |||
| public KnnVectorsReader getMergeInstance() { | |||
| public KnnVectorsReader getMergeInstance() throws IOException { | |||
There was a problem hiding this comment.
Somewhat tangential to this change:
I believe that getMergeInstance() initially didn't throw because the mental model was that it was akin to clone() but specialized for merging, and clone() doesn't throw an IOException. But we now want to run operations that may throw an IOException in getMergeInstance() impls. I think we need to decide if it's best to make all XXXFormat#getMergeInstance() throw an IOException (only a couple of them seem modified in this PR) or if impls should rethrow as an UncheckedIOException. (I don't thave an opinion on this question at this point.)
Another problem is that getMergeInstance() is no longer an independent clone (via other PRs, not yours), as the read advice is updated for everyone, not just for the merge instance. So it feels like this would be better done by a setter, to indicate that this affects the current instance.
There was a problem hiding this comment.
I think throwing an IOException is ok here - the top-level MergeState constructor already throws IOException, so no core semantics are being modified here. And it seems reasonable for a io-related method to throw an IOException. The alternative is to throw an UncheckedIOException, which may break the caller of MergeState. I can modify the other getMergeInstance methods to declare throws IOException for consistency here.
For the setter idea, that's a tricky one. The other getMergeInstance methods do create an independent instance, and there are also some implementations of KnnVectorsReader.getMergeInstance which do create a new instance (albeit to wrap other getMergeInstance objects). But we can't do that for Lucene99FlatVectorsReader, due to only having one underlying memory segment that we have to share between the IndexInput objects we use.
Ideally, we would clone - but we can't for this specific implementation. My hunch is that this is ok as-is, given we have AssertingKnnVectorsReader ensuring the calls happen as expected so that nothing breaks (I've tightened up the assertions for this). Any other changes in this area and we would need to re-evaluate though.
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog-check label to it and you will stop receiving this reminder on future updates to the PR. |
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog-check label to it and you will stop receiving this reminder on future updates to the PR. |
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
Update the
IOContextonIndexInputimplementations rather than theReadAdvice. This change means thatReadAdviceis now only used byMMapDirectoryand friends. Note that this is a refactoring only, it does not change behaviour.