hudi-agent commented on code in PR #19806:
URL: https://github.com/apache/hudi/pull/19806#discussion_r3901608577
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/table/action/commit/BaseSparkBucketIndexBucketInfoGetter.java:
##########
@@ -60,9 +69,21 @@ protected BucketInfo getBucketInfo(int bucketId, String
partitionPath) {
} else {
// Always write into log file instead of base file if using NB-CC
if (isNonBlockingConcurrencyControl) {
- return new BucketInfo(BucketType.UPDATE,
BucketIdentifier.newBucketFileIdForNBCC(bucketIdStr), partitionPath);
+ 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.
+ if (isFileGroupReplaced(partitionPath, fileId)) {
+ fileId = BucketIdentifier.newBucketFileIdForNBCC(bucketIdStr, 1);
Review 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?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/table/action/commit/SparkBucketIndexPartitioner.java:
##########
@@ -117,7 +122,21 @@ private void assignUpdates(WorkloadProfile profile) {
@Override
public SparkBucketInfoGetter getSparkBucketInfoGetter() {
return new SparkBucketIndexBucketInfoGetter(numBuckets, partitionPaths,
- updatePartitionPathFileIds, isOverwrite,
isNonBlockingConcurrencyControl);
+ updatePartitionPathFileIds, isOverwrite,
isNonBlockingConcurrencyControl, replacedPartitionFileIds);
+ }
+
+ private Map<String, Set<String>> computeReplacedFileIds(HoodieTable table) {
Review 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?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/table/action/commit/BaseSparkBucketIndexBucketInfoGetter.java:
##########
@@ -60,9 +69,21 @@ protected BucketInfo getBucketInfo(int bucketId, String
partitionPath) {
} else {
// Always write into log file instead of base file if using NB-CC
if (isNonBlockingConcurrencyControl) {
- return new BucketInfo(BucketType.UPDATE,
BucketIdentifier.newBucketFileIdForNBCC(bucketIdStr), partitionPath);
+ 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.
Review 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.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
