[core] Release every reader in SortMergeReaderWithMinHeap.close() - #9163
Open
PDGGK wants to merge 1 commit into
Open
[core] Release every reader in SortMergeReaderWithMinHeap.close()#9163PDGGK wants to merge 1 commit into
PDGGK wants to merge 1 commit into
Conversation
close() releases one RecordReader per sorted run being merged, across three loops -- nextBatchReaders, then the heap, then the polled elements. Every call was a bare close(), so the first one to throw abandoned the rest of its loop and both loops after it. Each of those readers holds an open data file, and this is the compaction path, so a single unreadable file strands every descriptor behind it for the remainder of the merge. Each step now runs and the first failure is the one that propagates, with later ones attached as suppressed, via ExceptionUtils .firstOrSuppressed -- the helper whose own javadoc gives this exact close-loop as its example. IOUtils.closeAll would have been the obvious choice but does not fit: it declares throws Exception, and both existing callers widened their own signatures to match, which this override of RecordReader.close() throws IOException cannot do. releaseBatch() is collected the same way. It declares no checked exception but can still throw, and it used to be able to skip the close() immediately after it. Three test cases: an earlier reader failing still closes the later ones and keeps its own exception as primary with the later failure suppressed; a failing releaseBatch() still closes both readers; and an all-succeed control that passes against both versions, so the other two are not trivially red. Against the previous close() the first and third fail.
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
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.
Purpose
Closes #9162.
SortMergeReaderWithMinHeap.close()releases oneRecordReaderper sorted run being merged, across three loops, and every call was a bareclose(). The first one to throw abandoned the rest of its loop and both loops after it.This is not a two-resource teardown: there is one reader per sorted run, each holds an open data file, and it runs on the compaction path. A single unreadable file therefore strands every descriptor behind it for the remainder of the merge.
element.iterator.releaseBatch()had the same exposure — no checked exception, but it can still throw, and when it did theclose()on the next line was skipped.Each step now runs, the first failure propagates, and later ones are attached to it as suppressed.
On the choice of helper.
IOUtils.closeAllis the obvious candidate and I did not use it: it declaresthrows Exception, and both existing callers (FormatTableFileWriter:93,FileChannelManagerImpl:126) widened their own signature to match. This method overridesRecordReader.close() throws IOExceptionand cannot widen.ExceptionUtils.firstOrSuppressedis the right fit — its javadoc gives this exact close-loop as its worked example.SortMergeReaderWithLoserTreeneeds no change; it delegates to a singleloserTree.close().Tests
New
SortMergeReaderWithMinHeapCloseTest, three cases:releaseBatch()throwsAgainst the previous
close(), the first and third of those fail; the all-succeed control passes on both versions, which is what shows the other two aren't trivially red.The readers are a small hand-rolled fake rather than mocks, so the test asserts on real call ordering through
readBatch()into the heap rather than on stub interactions.spotless:check,checkstyle:checkandapache-rat:checkare clean.Local run caveat. With
-Pfast-build,CompactionChangelogFollowUpScannerTestandMergeTreeCompactManagerTesterror in setup withCould not instantiate generated class 'RecordComparator'— the codegen module isn't built under that profile. I confirmed it's unrelated by revertingSortMergeReaderWithMinHeap.javato master and deleting the new test: both fail identically. Everything else in the merge-tree and compaction packages passes, includingMergeTreeReadersConnectionsLeakTest(20).