PDGGK opened a new pull request, #9172: URL: https://github.com/apache/paimon/pull/9172
### 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: | level | before | abandons | |---|---|---| | `SortLookupStoreReader.close()` | `reader.close()` → `input.close()` | **the file descriptor** | | `SstFileReader.close()` | `bloomFilter.close()` → `blockCache.close()` | every cached page of the file | | `BlockCache.close()` | `invalidPage(key)` per page, in a loop | the remaining pages | **The descriptor is never reclaimed.** Both callers — `LocalKvDb#closeAndDeleteSstFile` and the shutdown loop in `LocalKvDb#close` — catch `IOException` and 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.closeAll` does not fit here: it declares `throws Exception`, and all three of these implement or override a `close() throws IOException`. ### Tests `SortLookupStoreCloseTest` writes a real store to a temp file and drives it through a `CacheManager` subclass whose `invalidPage` can 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. | case | asserts | |---|---| | page invalidation fails | the block cache is still closed after the bloom filter fails (`invalidated > 1`), **and** the descriptor is released | | one page of several fails | every page is still handed back — `invalidated == pagesTaken` — not just those before the failure | | nothing fails | no exception; the control | Reverting each of the three fixes **one at a time** fails a different subset, so each level is pinned separately rather than as a block: | reverted | fails | |---|---| | `BlockCache` | `blockCacheHandsBackEveryPageWhenOneInvalidationFails` | | `SstFileReader` | `closeReleasesTheFileHandleWhenPageInvalidationFails` | | `SortLookupStoreReader` | both of the above | That 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. The `invalidated > 1` assertion was added specifically to close that gap. Existing suites pass: `SortLookupStoreTest` (12), `SortLookupStoreFactoryTest` (18), `CacheManagerTest` (2), `LocalDiskCacheManagerTest` (11). `spotless:check` and `checkstyle:check` exit 0. (`apache-rat:check` run standalone via `-pl paimon-common` reports 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.) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
