[BUG] Fix delete_partition followed by re-insert silently loses data under NBCC - #19806
[BUG] Fix delete_partition followed by re-insert silently loses data under NBCC#19806zhang-arvin wants to merge 1 commit into
Conversation
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR addresses silent data loss under NBCC when delete_partition is followed by a re-insert, by bumping the NBCC file-id generation when the default file group has been retired by a replacecommit. The mechanism is sound for a single delete/re-insert cycle, but there's one edge case worth double-checking in the inline comment around repeated cycles. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. Two small issues worth tidying: a duplicated private method across both partitioner classes, and a magic-number generation bump.
| // (e.g. delete_partition), bump the generation so the new file group id | ||
| // no longer matches the retired one, avoiding silent data loss. | ||
| if (isFileGroupReplaced(partitionPath, fileId)) { | ||
| fileId = BucketIdentifier.newBucketFileIdForNBCC(bucketIdStr, 1); |
There was a problem hiding this comment.
🤖 Hardcoding the bump to generation 1 seems to only cover a single delete_partition + re-insert cycle. On a second cycle, delete_partition retires the now-active gen1, so getAllReplacedFileGroups returns {gen0, gen1} — then this re-inserts to gen1 again, which is also retired, and we're back to silent data loss. Could you loop to the first generation that isn't in the replaced set instead, e.g. int gen = 0; String fileId = newBucketFileIdForNBCC(bucketIdStr, gen); while (isFileGroupReplaced(partitionPath, fileId)) { fileId = newBucketFileIdForNBCC(bucketIdStr, ++gen); }? @nsivabalan does that match your read of how repeated delete_partition cycles accumulate in the replaced set under NBCC?
| updatePartitionPathFileIds, isOverwrite, isNonBlockingConcurrencyControl, replacedPartitionFileIds); | ||
| } | ||
|
|
||
| private Map<String, Set<String>> computeReplacedFileIds(HoodieTable table) { |
There was a problem hiding this comment.
🤖 nit: computeReplacedFileIds is copy-pasted verbatim into both partitioner classes — could you move it up to the base class (which already owns replacedPartitionFileIds) or extract it into a small static helper so there's only one copy to maintain?
| String fileId = BucketIdentifier.newBucketFileIdForNBCC(bucketIdStr); | ||
| // When the default generation file group has been retired by a replacecommit | ||
| // (e.g. delete_partition), bump the generation so the new file group id | ||
| // no longer matches the retired one, avoiding silent data loss. |
There was a problem hiding this comment.
🤖 nit: the literal 1 is a bit opaque — a named constant like NEXT_GENERATION (or even a brief inline comment clarifying it's always generation 0 + 1) would make the intent clearer to a future reader.
3 participants
Fixes #19793: Under Non-Blocking Concurrency Control, delete_partition followed by re-insert results in silent data loss.
Root Cause
NBCC uses fixed file IDs per bucket. After delete_partition retires the file group, re-inserts use the same fixed file ID, but FileSystemView filters out the retired file group, causing data to be written but invisible.
Fix
When the fixed file ID has been retired by a replacecommit, bump the generation to create a new file ID that no longer matches the retired file group.