Move finishMerge to a finally block so it always runs #14977
Conversation
github-project-automation
bot
moved this to Open
in OpenSearch Lucene & Core Performance Tracking
github-actions
bot
added
module:core/index
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 label to it and you will stop receiving this reminder on future updates to the PR. |
thecoop
added
the
skip-changelog
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution! |
There was a problem hiding this comment.
I can't pretend I fully understand the changes introduced but I get the intent and it seems ok. I left one comment where the test seemed to be intentionally calling close twice. I'd beast it locally a bit but if something is wrong in the logic here, I'm sure it'll pop up in tests.
...t-framework/src/java/org/apache/lucene/tests/codecs/asserting/AssertingKnnVectorsFormat.java
Show resolved
Hide resolved
|
The logic seems correct - I'm just not sure why we're doing it? Something to do with soft deletes? |
|
OK, I think I am concerned about the pre-existing method -- shouldn't it restore to the prior state rather than unconditionally setting a specific advice? But that is really out of scope of this PR, which seems fine |
|
See #14844 for properly refactoring how that method works |
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution! |
There was a problem hiding this comment.
LGTM, although I'm not that intimate with IndexWriter internals. @mikemccand would be a good person to weigh in.
| @@ -225,7 +226,7 @@ private void mergeTerms(SegmentWriteState segmentWriteState, SegmentReadState se | |||
| } | |||
| } | |||
|
|
|||
| public void mergeFieldInfos() { | |||
| private void mergeFieldInfos() { | |||
There was a problem hiding this comment.
This hides a method that was previous public. Is this intentional (people do all kinds of low-level hacks with segments).
There was a problem hiding this comment.
All the other merge methods are private, and from the git blame there is no particular reason for this, so this is just some consolidation. I can revert it if there's a possibility for calling this specific method elsewhere?
There was a problem hiding this comment.
I've no idea - I'm not using it. But I know folks downstream sometimes tinker with super low level stuff... We can try to make it private and see if anybody shouts out. :)
Follow on from #14930. Turns out a full
closeimplementation is not possible, as the default return value fromgetMergeInstanceisthis, requiring reference counting or other mechanisms to make sure the correct thing gets closed. So instead, I've moved some of the responsibilities around, and made sure thatfinishMergeis always called regardless of merge outcome.Resolves #14930