Reapply "Update the IOContext rather than the ReadAdvice on IndexInput (#14702)" #14844
Conversation
apache#14702)" This reverts commit cf937c6.
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 label to it and you will stop receiving this reminder on future updates to the PR. |
|
Don't know if you're subscribed - Tests are randomized - sometimes a different reshuffle of components may not succeed after a patch. Here's an example: I haven't tried if it reproduces or not - I'm very limited today - seems like the builds have gone back to normal after I reverted the patch though, so something is not right? |
|
Yes, that's caused by this PR... It reproduces, but only intermittently... |
|
I get a high failure ratio with this: |
|
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. |
| @@ -228,8 +245,6 @@ public Map<String, Long> getOffHeapByteSize(FieldInfo fieldInfo) { | |||
|
|
|||
| @Override | |||
| public void close() throws IOException { | |||
| assert !mergeInstance; | |||
| delegate.close(); | |||
| delegate.close(); | |||
| assert finishMergeCount.get() <= 0 || mergeInstanceCount.get() == finishMergeCount.get(); | |||
There was a problem hiding this comment.
This was the line causing the problems (which was modified in the original PR) - its ok for close() to be called when there's an open merge instance, if the merge is aborted - which is what testNoWaitClose is doing
There was a problem hiding this comment.
I'm not sitting that deep in codecs. @jpountz is much better fitted for the review of this.
There was a problem hiding this comment.
I would expect Lucene to never close merge instances, since merge instances are conceptually like clones, and Lucene doesn't close clones.
That said, it looks like we should make sure that finishMerge is always called - even if the merge fails or is aborted, otherwise aborted merges would leave readers open with the wrong read advice?
There was a problem hiding this comment.
This is an issue with the code calling finishMerge, not in this PR. Shall I merge this change, then look at pinning down the missing finishMerge separately?
There was a problem hiding this comment.
I've found the cause here
Reapply #14702