jpountz commented on code in PR #14702:
URL: https://github.com/apache/lucene/pull/14702#discussion_r2102989752
##########
lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsReader.java:
##########
@@ -124,7 +124,7 @@ public abstract void search(
*
* <p>The default implementation returns {@code this}
*/
- public KnnVectorsReader getMergeInstance() {
+ public KnnVectorsReader getMergeInstance() throws IOException {
Review 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.
--
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: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
