Core: concurrent manifest filtering corrupts deleteFiles on parallel overwrite#16981
Open
goingforstudying-ctrl wants to merge 5 commits into
Open
Conversation
57d5b90 to
a2553b9
Compare
Was debugging a ClassCastException that kept popping up during overwrite operations on tables with many manifests. Took a while to trace it back to the parallel manifest filtering path. The issue is that filterManifests() uses Tasks.range() to process manifests concurrently across worker threads. Inside filterManifestWithDeletedFiles(), worker threads were directly mutating the shared deleteFiles LinkedHashSet via deleteFiles.add(fileCopy). LinkedHashSet is not thread-safe, so concurrent adds from multiple threads corrupt the internal HashMap, producing the ClassCastException: LinkedHashMap cannot be cast to HashMap that was reported in apache#16978. The fix has three parts: 1. Collect per-manifest deleted files into a ConcurrentMap from each worker thread instead of mutating the shared set. After all parallel filtering completes, merge the per-manifest results into deleteFiles on the main thread. 2. Replace the filteredManifestToDeletedFiles HashMap with a ConcurrentHashMap so worker threads can safely put their results without resize races. 3. Add ThreadSafeFileSet, a ConcurrentHashMap-backed Set implementation for defense-in-depth. Use it for the deleteFiles collection so even if future code accidentally adds concurrent mutations, the set won't corrupt. I also switched filteredManifestToDeletedFiles to ConcurrentHashMap because worker threads write to it from filterManifest() while the main thread reads after join. The old plain HashMap could resize unpredictably under concurrent writes. Added TestManifestFilterManagerConcurrency with two tests that create tables with 32 manifests and perform full overwrites/deletes to exercise the parallel filtering path. Closes apache#16978
- Add @parameter annotations and parameters() method for parameterized tests - Replace @test with @testtemplate for parameterized test extension - Replace non-existent deleteByRowFilter with deleteFromRowFilter - Replace non-existent deletedDataFiles with removedDataFiles - Add missing import for java.util.stream.Stream These changes fix compilation errors against the current Iceberg API.
17ae08e to
3a71d1a
Compare
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.
Was debugging a ClassCastException that kept popping up during overwrite operations on tables with many manifests. Took a while to trace it back to the parallel manifest filtering path.
The issue is that
filterManifests()usesTasks.range()to process manifests concurrently across worker threads. InsidefilterManifestWithDeletedFiles(), worker threads were directly mutating the shareddeleteFilesLinkedHashSet viadeleteFiles.add(fileCopy). LinkedHashSet is not thread-safe, so concurrent adds from multiple threads corrupt the internal HashMap, producing theClassCastException: LinkedHashMap$Entry cannot be cast to HashMap$TreeNodethat was reported in #16978.The fix has three parts:
Collect per-manifest deleted files into a
ConcurrentMapfrom each worker thread instead of mutating the shared set. After all parallel filtering completes, merge the per-manifest results intodeleteFileson the main thread.Replace the
filteredManifestToDeletedFilesHashMap with aConcurrentHashMapso worker threads can safely put their results without resize races.Add
ThreadSafeFileSet, aConcurrentHashMap-backedSetimplementation for defense-in-depth. Use it for thedeleteFilescollection so even if future code accidentally adds concurrent mutations, the set won't corrupt.I also switched
filteredManifestToDeletedFilestoConcurrentHashMapbecause worker threads write to it fromfilterManifest()while the main thread reads after join. The old plain HashMap could resize unpredictably under concurrent writes.Added
TestManifestFilterManagerConcurrencywith two tests that create tables with 32 manifests and perform full overwrites/deletes to exercise the parallel filtering path.Not 100% sure if
ThreadSafeFileSetshould live inapiorcore— went withapibecause it's a general utility, but happy to move it if that doesn't feel right.Closes #16978