fix(flink): close CDC image spillable maps on failures - #19482
Conversation
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR tightens resource cleanup in the Flink CDC image cache — closing newly-allocated spillable maps when iterator creation or traversal fails, replacing the unclosed empty spillable map in the absent-before-slice path with Collections.emptyMap(), and closing both the log-record iterator and image manager while preserving the primary failure. I traced the mutation, failure, and close paths (including that the immutable empty map is never written, since updateImageRecord is unreachable when removeImageRecord always returns null) and everything holds up. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. One small naming nit in the new helper method; otherwise the change is clean and readable.
cc @yihua
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19482 +/- ##
============================================
+ Coverage 76.96% 76.99% +0.02%
- Complexity 33850 33866 +16
============================================
Files 2575 2576 +1
Lines 143372 143420 +48
Branches 17572 17645 +73
============================================
+ Hits 110349 110423 +74
+ Misses 24758 24742 -16
+ Partials 8265 8255 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
voonhous
left a comment
voonhous
left a comment
There was a problem hiding this comment.
Reviewed the lifecycle changes against how CdcImageManager is actually shared across splits, not just the diff.
The core fix in loadImageRecords is right, and the Collections.emptyMap() swap is a genuine improvement -- I verified on JDK 8/17/21 that Collections.emptyMap().remove(k) returns null through the inherited AbstractMap.remove (EmptyMap does not override the 1-arg form), so the unreachability argument for updateImageRecord holds, and the old else-branch map was never written to and never registered in the cache, i.e. a real per-split leak since #11490. No accidental revert: #19402 (46cd13434308) and #19202 (d10b868d90c9) are both intact on this branch.
Two things I would want resolved before merge, both inline: the close() in DataLogFileIterator is silently load-bearing for correctness and needs a comment, and the suppressed-exception behavior the PR body advertises has no test. Also inline: two more instances of this PR's own leak class in the surrounding constructors, and CdcImageManager.close() itself. The rest are cleanliness.
github-actions
Bot
added
size:M
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR tightens resource cleanup on the Flink CDC read path: it closes newly-allocated spillable maps and log-record iterators when image loading or iterator construction fails, moves CdcImageManager ownership entirely to CdcFileSplitsIterator, and uses Collections.emptyMap() for the absent before-slice. I traced the ownership/lifecycle model and failure paths in detail: removing imageManager.close() from DataLogFileIterator.close() correctly fixes a premature-close of the shared manager, the Collections.emptyMap() path is safe (the updateImageRecord mutation branch is unreachable when the before-slice is absent), and the suppressed-exception cleanup is idempotent. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here.
. One duplication suggestion around the repeated closeSuppressing helper; otherwise clean.
cc @yihua
github-actions
Bot
added
size:L
Pull request was closed
CDC iterators leaked ExternalSpillableMap instances when construction or iteration failed: the image manager was never closed on the failure path, leaving spill files behind. Route cleanup through a shared CloseableUtils.closeSuppressing helper, close the image manager via try-with-resources, and retain CDC images across child splits. (cherry picked from commit f41e8e3) Cherry-pick adaptations: - HoodieSplitReaderFunction: the private closeSuppressing helper being deleted is typed HoodieFileGroupReader<RowData> here rather than HoodieRecordReader<RowData>, since the LSM reader refactor (#18987, #19079, #19307) is not on this branch. Same deletion, different pre-existing signature. The shared replacement takes AutoCloseable and HoodieFileGroupReader implements Closeable, so call sites are unchanged. - CdcIterators: upstream drops the FormatUtils import and keeps HoodieRowDataFileReader / InternalSchemaManager as context. This branch's copy of the file never imported the latter two and does not reference them, so only the FormatUtils import is dropped. Adding the other two would be unused imports and fail checkstyle. The static import of FormatUtils.buildAvroRecordBySchema is unaffected. - Dropped the TestCdcImageManager and TestCdcIterators changes. Both test classes arrive with #19402, which is not on this branch, and both assert behavior from that commit's production half: TestCdcImageManager expects skipBytesToRead to throw EOFException, which without #19402 loops forever rather than failing. Importing them would add a hanging test. The fix therefore lands without its CDC test coverage. TestCloseableUtils is included, so the shared helper itself is covered.
CDC iterators leaked ExternalSpillableMap instances when construction or iteration failed: the image manager was never closed on the failure path, leaving spill files behind. Route cleanup through a shared CloseableUtils.closeSuppressing helper, close the image manager via try-with-resources, and retain CDC images across child splits. (cherry picked from commit f41e8e3) Cherry-pick adaptations: - HoodieSplitReaderFunction: the private closeSuppressing helper being deleted is typed HoodieFileGroupReader<RowData> here rather than HoodieRecordReader<RowData>, since the LSM reader refactor (#18987, #19079, #19307) is not on this branch. Same deletion, different pre-existing signature. The shared replacement takes AutoCloseable and HoodieFileGroupReader implements Closeable, so call sites are unchanged. - CdcIterators: upstream drops the FormatUtils import and keeps HoodieRowDataFileReader / InternalSchemaManager as context. This branch's copy of the file never imported the latter two and does not reference them, so only the FormatUtils import is dropped. Adding the other two would be unused imports and fail checkstyle. The static import of FormatUtils.buildAvroRecordBySchema is unaffected. - Dropped the TestCdcImageManager and TestCdcIterators changes. Both test classes arrive with #19402, which is not on this branch, and both assert behavior from that commit's production half: TestCdcImageManager expects skipBytesToRead to throw EOFException, which without #19402 loops forever rather than failing. Importing them would add a hanging test. The fix therefore lands without its CDC test coverage. TestCloseableUtils is included, so the shared helper itself is covered.
5 participants
Describe the issue this Pull Request addresses
CDC image loading allocates spillable maps that were not closed when iterator creation or traversal failed, leaking disk and native resources. The LOG_FILE path also allocated a spillable map for an absent before-slice even though that cache remains empty.
Summary and Changelog
Mapinterface and useCollections.emptyMap()when no before-image slice exists.Impact
Prevents temporary disk and native resource leaks in Flink CDC reads. There are no public API, configuration, storage-format, or user-visible behavior changes.
Risk Level
Low. The changes are localized to Flink CDC image-cache lifecycle handling and are covered by focused unit tests.
Documentation Update
None.
Contributor's checklist