voonhous commented on issue #19881:
URL: https://github.com/apache/hudi/issues/19881#issuecomment-5646740932
Three more defects in `unscheduleCompactionFileId`, found while reviewing
#19924. None are introduced by that PR and none gate it; recording them here so
the method gets cleaned up in one pass rather than piecemeal.
**NPE for a file group with no pending compaction**
(`CompactionAdminClient.java:145-146`)
```java
Pair<String, HoodieCompactionOperation> compactionOperationWithInstant =
CompactionUtils.getAllPendingCompactionOperations(metaClient).get(fgId);
HoodieCompactionPlan plan =
CompactionUtils.getCompactionPlan(metaClient,
compactionOperationWithInstant.getKey());
```
`.get(fgId)` returns null for an unknown file group and the next line
dereferences it. `HoodieFileGroupId.equals` compares both partition path and
file id, and hudi-cli's `--partitionPath` defaults to `""`
(`CompactionCommand.java:553`), so `compaction unscheduleFileId --fileId X` on
a partitioned table without an explicit partition gives the operator a raw NPE.
The guard that used to throw `HoodieException("FileGroupId " + fgId + " not in
pending compaction")` lived in
`getRenamingActionsForUnschedulingCompactionForFileId`, deleted by 0d8554f95a81
(#9776); `grep -rn 'not in pending compaction' --include='*.java' .` returns
nothing at master.
**Unscheduling the last operation leaves a plan that can never complete**
(`CompactionAdminClient.java:149-167`)
When `newOps` comes out empty the method still writes the plan back, leaving
a zero-operation `.compaction.requested` on the timeline.
`HoodieCompactor.compact` short-circuits on an empty operations list
(`HoodieCompactor.java:90-93`) and returns before
`transitionCompactionRequestedToInflight`, but `RunCompactionActionExecutor`
goes on to build commit metadata, and
`CompactHelpers.completeInflightCompaction` then trips
`ValidationUtils.checkArgument(storage.exists(<instant>.compaction.inflight),
"File ... does not exist!")` at `ActiveTimelineV2.java:560-562`. The instant is
stuck pending and blocks archival.
The sibling API handles this by deleting the requested instant instead
(`unscheduleCompactionPlan`, lines 119-123). #19924 narrows the exposure a lot
(before it, unscheduling any file group in a single-partition plan emptied the
plan) but does not close it.
**Inflight compactions are silently reverted instead of refused**
(`CompactionAdminClient.java:158-162`)
`unscheduleCompactionPlan` refuses outright with
`IllegalStateException("Please rollback the inflight compaction before
unscheduling")` (line 113), while `unscheduleCompactionFileId` calls
`revertInstantFromInflightToRequested`, which just deletes the inflight file,
and rewrites the plan. Any base files the inflight run already wrote are left
orphaned. The branch is reached by no test: every
`setupAndValidateCompactionOperations` call in `TestCompactionAdminClient`
passes `inflight=false`.
**Archaeology on the original filter bug, for whoever picks this up**
The inverted predicate was introduced by 3ae6cb4ed582 (2019-02-12), which
replaced the correct single-condition `!op.getFileId().equals(fileId)` with
`(!fileId.equals) && (!partition.equals)` while making the admin client
partition-aware. Until 1.0.0 the damage was mostly masked: the rewrite was
guarded by `if (!dryRun && !res.isEmpty() && res.get(0).isExecuted() &&
res.get(0).isSuccess())`, so with no log files to rename `res` was empty and
the rewrite was skipped. 0d8554f95a81 (#9776) relaxed that to a plain `if
(!dryRun)`, at which point the buggy rewrite ran on every invocation. The
predicate is present verbatim on `release-1.2.0` and `release-1.1.1`, so #19924
is a 1.2.1 backport candidate.
--
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]
