[common] Finish the lookup-store close chain instead of stopping at the first failure - #9172
Open
PDGGK wants to merge 1 commit into
Open
[common] Finish the lookup-store close chain instead of stopping at the first failure#9172PDGGK wants to merge 1 commit into
PDGGK wants to merge 1 commit into
Conversation
…he first failure Closing a sorted lookup store walks three levels, and each was a bare sequence of calls: SortLookupStoreReader.close() reader.close() -> input.close() SstFileReader.close() bloomFilter.close() -> blockCache.close() BlockCache.close() cacheManager.invalidPage(key) per cached page So one failing page invalidation at the bottom abandons the remaining pages, then the block cache, then the file handle two levels up. input is the descriptor. That descriptor is never reclaimed. Both callers -- LocalKvDb #closeAndDeleteSstFile and the shutdown loop in LocalKvDb#close -- deliberately catch and log, so that one bad reader cannot stall shutdown. Which is right, and is exactly why an abandoned descriptor here surfaces only as a warning line and nothing ever retries it. Every step now runs, the first failure propagates, and later ones are attached as suppressed, using ExceptionUtils.firstOrSuppressed as in SortMergeReaderWithMinHeap. IOUtils.closeAll does not fit: it declares throws Exception and all three of these override or implement a close() throws IOException. SortLookupStoreCloseTest builds a real store over a temp file and drives it with a CacheManager whose invalidPage can be made to fail. Reverting the three fixes one at a time fails a different subset each time, so each level is pinned separately rather than as a block.
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 #9171.
Closing a sorted lookup store walks three levels, each a bare sequence of calls, so one failing page invalidation at the bottom abandons everything above it:
SortLookupStoreReader.close()reader.close()→input.close()SstFileReader.close()bloomFilter.close()→blockCache.close()BlockCache.close()invalidPage(key)per page, in a loopThe descriptor is never reclaimed. Both callers —
LocalKvDb#closeAndDeleteSstFileand the shutdown loop inLocalKvDb#close— catchIOExceptionand log a warning so that one bad reader cannot stall shutdown. That is correct behaviour, and it is precisely why an abandoned descriptor here surfaces only as a log line with nothing to retry it. Readers are cached per SST file, so it accumulates.Every step now runs, the first failure propagates, and later ones ride along as suppressed, via
ExceptionUtils.firstOrSuppressed— the same helper used in #9163, and the one whose javadoc gives this close-loop as its worked example.IOUtils.closeAlldoes not fit here: it declaresthrows Exception, and all three of these implement or override aclose() throws IOException.Tests
SortLookupStoreCloseTestwrites a real store to a temp file and drives it through aCacheManagersubclass whoseinvalidPagecan be made to fail — no mocking framework, and the file handle is a small recording wrapper so the assertion is on the real thing being closed.invalidated > 1), and the descriptor is releasedinvalidated == pagesTaken— not just those before the failureReverting each of the three fixes one at a time fails a different subset, so each level is pinned separately rather than as a block:
BlockCacheblockCacheHandsBackEveryPageWhenOneInvalidationFailsSstFileReadercloseReleasesTheFileHandleWhenPageInvalidationFailsSortLookupStoreReaderThat middle row is worth noting: my first version of the test did not pin
SstFileReader— reverting it broke nothing, because the assertion I had was already protected by the outermost fix. Theinvalidated > 1assertion was added specifically to close that gap.Existing suites pass:
SortLookupStoreTest(12),SortLookupStoreFactoryTest(18),CacheManagerTest(2),LocalDiskCacheManagerTest(11).spotless:checkandcheckstyle:checkexit 0.(
apache-rat:checkrun standalone via-pl paimon-commonreports 4 unapproved files —LICENSE.antlr-runtime,LICENSE.janino,JavaLexer.g4,JavaParser.g4. All pre-existing and untouched here; the standalone invocation bypasses the exclusions configured in the parent build.)