Simplify DerivedSourceReaders lifecycle by removing manual ref-counting#3138
Open
shatejas wants to merge 9 commits intoopensearch-project:mainfrom
Open
Simplify DerivedSourceReaders lifecycle by removing manual ref-counting#3138shatejas wants to merge 9 commits intoopensearch-project:mainfrom
shatejas wants to merge 9 commits intoopensearch-project:mainfrom
Conversation
Reference counting for the underlying readers (StoredFieldsReader in this case) is already handled by Lucene's SegmentReader, so DerivedSourceReaders does not need its own AtomicInteger ref-count. StoredFieldsReader.clone() is called by Lucene on every document read (e.g. get/_source fetch), so the cloned instance must not close the underlying readers — only the owning instance should. This is achieved by delegating close behavior to a Closeable onClose: the public constructor registers a real close action, while clone() and getMergeInstance() produce non-owning instances with a no-op close. Signed-off-by: Tejas Shah <shatejas@amazon.com>
Signed-off-by: Tejas Shah <shatejas@amazon.com>
Signed-off-by: Tejas Shah <shatejas@amazon.com>
shatejas
requested review from
0ctopus13prime,
VijayanB,
Vikasht34,
heemin32,
jmazanec15,
junqiu-lei,
luyuncheng,
martin-gaievski,
naveentatikonda,
navneet1v,
ryanbogan and
vamshin
as code owners
Signed-off-by: Tejas Shah <shatejas@amazon.com>
5 tasks
Signed-off-by: Tejas Shah <shatejas@amazon.com>
Signed-off-by: Tejas Shah <shatejas@amazon.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3138 +/- ##
============================================
+ Coverage 82.42% 82.51% +0.08%
- Complexity 3867 3880 +13
============================================
Files 418 418
Lines 14462 14472 +10
Branches 1850 1850
============================================
+ Hits 11921 11941 +20
+ Misses 1780 1771 -9
+ Partials 761 760 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Navneet Verma <navneev@amazon.com>
navneet1v
previously approved these changes
Mar 12, 2026
Signed-off-by: Tejas Shah <shatejas@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Reference counting for the underlying readers (StoredFieldsReader in this case) is already handled by Lucene's SegmentReader, so DerivedSourceReaders does not need its own ref-count.
StoredFieldsReader.clone() is called by Lucene on every document read (e.g. get/_source fetch), so the cloned instance must not close the underlying readers — only the owning instance should. This is achieved by delegating close behavior to a Closeable onClose: the public constructor registers a real close action, while clone() and getMergeInstance() produce non-owning instances with a no-op close.
Related Issues
Resolves #OpenSearch#20622
Repro attempt and logs
I modified the code and added some logs to understand when the readers where getting closed. For certain conditions freeReaderContext tries to close the OpensearchReaderManager on a cloned instance of storedfieldsReader. Ideally this should not happen but there seems to a race condition hard to pinpoint on where exactly the ref turns 0 and it tries to close the underlying readers.
The clone implementation is now consistent with Lucene90CompressingStoredFieldsReader.java
Testing
The change was tested using reindex(calls freeReaderContext underneath) and force merge operations to make sure it works
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.