voonhous commented on code in PR #19924:
URL: https://github.com/apache/hudi/pull/19924#discussion_r3996624149
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/CompactionAdminClient.java:
##########
@@ -147,7 +147,7 @@ public List<RenameOpResult>
unscheduleCompactionFileId(HoodieFileGroupId fgId, b
HoodieCompactionPlan plan =
CompactionUtils.getCompactionPlan(metaClient,
compactionOperationWithInstant.getKey());
List<HoodieCompactionOperation> newOps =
plan.getOperations().stream().filter(op ->
- (!op.getFileId().equals(fgId.getFileId())) &&
(!op.getPartitionPath().equals(fgId.getPartitionPath())))
+ !(op.getFileId().equals(fgId.getFileId()) &&
op.getPartitionPath().equals(fgId.getPartitionPath())))
Review Comment:
**blocker:** this flips the behaviour that
`TestCompactionCommand.testSparkMainCompactUnscheduleFile` currently pins, and
the PR does not update it, so `FT - hudi-cli` will fail. The `dryRun=false` arm
at `TestCompactionCommand.java:287` asserts `Collections.emptySet()`, and its
comment names this issue: "When that is fixed this expectation has to become
the sibling on its own."
Could we update that arm here, so the fix and its expectation land together?
```java
} else {
Set<String> expected = fileIdsBefore.stream()
.filter(fileId ->
!fileId.equals(unscheduled.getFileId())).collect(Collectors.toSet());
assertEquals(expected, fileIdsOf(PENDING_COMPACTION_INSTANT));
}
```
(`Collections` stays imported; it is still used at line 344.)
##########
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/TestCompactionAdminClient.java:
##########
@@ -83,6 +86,35 @@ public void testUnscheduleCompactionPlan() throws Exception {
validateUnSchedulePlan(client, "006", "007", numEntriesPerInstant);
}
+ @Test
+ public void testUnscheduleCompactionFileIdKeepsSiblingOpsInSamePartition()
throws Exception {
+ int numOpsInPlan = 3;
+ CompactionTestUtils.setupAndValidateCompactionOperations(metaClient,
false, numOpsInPlan,
+ 0, 0, 0);
+ String compactionInstant = "001";
+ HoodieCompactionPlan planBefore =
CompactionUtils.getCompactionPlan(metaClient, compactionInstant);
+ assertEquals(numOpsInPlan, planBefore.getOperations().size());
Review Comment:
**nit:** feel free to ignore. `setupAndValidateCompactionOperations` already
asserts the per-plan operation count (`CompactionTestUtils.java:105-113`) and
that the timeline read matches, so this line cannot fail on its own. Could we
drop it and keep `planBefore` only for deriving the sibling set?
##########
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/TestCompactionAdminClient.java:
##########
@@ -83,6 +86,35 @@ public void testUnscheduleCompactionPlan() throws Exception {
validateUnSchedulePlan(client, "006", "007", numEntriesPerInstant);
}
+ @Test
+ public void testUnscheduleCompactionFileIdKeepsSiblingOpsInSamePartition()
throws Exception {
+ int numOpsInPlan = 3;
+ CompactionTestUtils.setupAndValidateCompactionOperations(metaClient,
false, numOpsInPlan,
Review Comment:
**major:** the new test pins only the fileId half of the predicate.
`CompactionTestUtils.createCompactionPlan` hard-codes
`DEFAULT_PARTITION_PATHS[0]` for every op, so a mutant that drops the partition
check entirely, `filter(op -> !op.getFileId().equals(fgId.getFileId()))`, still
passes this test.
That arm is real in production: with bucket index under NBCC,
`BucketIdentifier.newBucketFileIdPrefix(bucketId, true)` returns `bucketIdStr +
CONSTANT_FILE_ID_SUFFIX`, so the same fileId appears in every partition. Could
we add an operation carrying the target fileId under a different partition path
and assert it survives?
--
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]
