hudi-agent commented on code in PR #19226:
URL: https://github.com/apache/hudi/pull/19226#discussion_r3579634165
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/ListingBasedRollbackStrategy.java:
##########
@@ -151,20 +155,25 @@ public List<HoodieRollbackRequest>
getRollbackRequests(HoodieInstant instantToRo
// as part of regular rollback(on re-attempting a failed
compaction), we might have to delete/rollback only the base file that could have
// potentially been created. Even if there are log files added
to the file slice of interest, we should not touch them.
// but if its part of a restore operation, rolling back a
compaction should rollback entire file slice, i.e base file and all log files.
- if (!isRestore) {
- // Rollback of a compaction action if not for restore means
that the compaction is scheduled
- // and has not yet finished. In this scenario we should delete
only the newly created base files
- // and not corresponding base commit log files created with
this as baseCommit since updates would
- // have been written to the log files.
-
hoodieRollbackRequests.addAll(getHoodieRollbackRequests(partitionPath,
-
listBaseFilesToBeDeleted(instantToRollback.requestedTime(), baseFileExtension,
partitionPath, metaClient.getStorage())));
- } else {
+ if (isRestore) {
// if this is part of a restore operation, we should
rollback/delete entire file slice.
// For table version 6, the files can be directly fetched from
the instant to rollback
// For table version 8, the log files are not directly
associated with the base file.
// The rollback will iterate in reverse order based on
completion time so the log files completed
// after the compaction will already be queued for removal and
therefore, only the files from the compaction commit must be deleted.
hoodieRollbackRequests.addAll(getHoodieRollbackRequests(partitionPath,
filesToDelete.get()));
+ } else {
Review Comment:
🤖 nit: this `else { if (...) }` adds an extra nesting level on top of the
switch/if — could you collapse it into `else if (hasLaterDeltaCommits(...))` to
flatten it a bit? The two comment blocks move cleanly onto each branch.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/ListingBasedRollbackStrategy.java:
##########
@@ -276,6 +286,15 @@ public static List<HoodieRollbackRequest>
getRollbackRequestToAppendForVersionSi
return hoodieRollbackRequests;
}
+ private static boolean hasLaterDeltaCommits(HoodieActiveTimeline
activeTimeline, HoodieInstant instantToRollback) {
+ return !activeTimeline.getDeltaCommitTimeline()
+ .findInstantsAfter(instantToRollback.requestedTime(), 1).empty();
+ }
+
+ private static List<String>
getPartitionPathsFromCommitMetadata(HoodieCommitMetadata commitMetadata) {
Review Comment:
🤖 nit: this helper just wraps `new
ArrayList<>(commitMetadata.getWritePartitionPaths())` for a single caller —
might be simpler to inline it at the ternary rather than add a static method.
<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]
