hudi-agent commented on code in PR #19226:
URL: https://github.com/apache/hudi/pull/19226#discussion_r3569550851
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/RollbackToInstantTimeProcedure.scala:
##########
@@ -65,24 +74,65 @@ class RollbackToInstantTimeProcedure extends BaseProcedure
with ProcedureBuilder
.setLayoutVersion(Option.of(new
TimelineLayoutVersion(config.getTimelineLayoutVersion)))
.build
- val activeTimeline = metaClient.getActiveTimeline
+ val activeTimeline =
metaClient.getActiveTimeline.getCommitsAndCompactionTimeline
val filteredTimeline = activeTimeline.containsInstant(instantTime)
if (!filteredTimeline) {
throw new HoodieException(s"Commit $instantTime not found in Commits
$activeTimeline")
}
- val result = if (client.rollback(instantTime)) true else false
+ val res = new util.ArrayList[Row]()
Review Comment:
🤖 nit: `res` is pretty opaque — could you rename it to `rollbackResults` (or
even just `results`) so it's clear what the list is accumulating?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/RollbackToInstantTimeProcedure.scala:
##########
@@ -65,24 +74,65 @@ class RollbackToInstantTimeProcedure extends BaseProcedure
with ProcedureBuilder
.setLayoutVersion(Option.of(new
TimelineLayoutVersion(config.getTimelineLayoutVersion)))
.build
- val activeTimeline = metaClient.getActiveTimeline
+ val activeTimeline =
metaClient.getActiveTimeline.getCommitsAndCompactionTimeline
val filteredTimeline = activeTimeline.containsInstant(instantTime)
if (!filteredTimeline) {
throw new HoodieException(s"Commit $instantTime not found in Commits
$activeTimeline")
}
- val result = if (client.rollback(instantTime)) true else false
+ val res = new util.ArrayList[Row]()
+ val instantsToRollback =
activeTimeline.findInstantsAfterOrEquals(instantTime, Integer.MAX_VALUE)
+ .getInstants.asScala.reverse
+ breakable {
+ instantsToRollback.foreach { currentInstant =>
+ var finished = false
Review Comment:
🤖 nit: `finished` reads more like a loop-control flag than a success
indicator — `succeeded` (or `rollbackSucceeded`) would match how it's actually
used and make the `if (!finished) { break() }` guard a bit clearer.
<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:
##########
@@ -151,20 +155,28 @@ 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 {
+ HoodieActiveTimeline activeTimeline =
metaClient.getActiveTimeline();
+ boolean higherDeltaCommits =
!activeTimeline.getDeltaCommitTimeline()
+ .findInstantsAfter(instantToRollback.requestedTime(),
1).empty();
+ if (higherDeltaCommits) {
Review Comment:
🤖 When a later delta commit exists, this deletes the compaction's base file
but keeps log files that carry the compaction instant as their base commit. If
that delta commit is already completed, doesn't this leave a log-only slice
pointing at a base instant with no base file (whose updates can no longer merge
onto anything)? `rollback_to_instant` rolls back later commits first so it
won't hit this branch, but a direct out-of-order `client.rollback(compaction)`
could — is this path only intended for pending/inflight later delta commits?
<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 +289,10 @@ public static List<HoodieRollbackRequest>
getRollbackRequestToAppendForVersionSi
return hoodieRollbackRequests;
}
+ private List<String> getPartitionPathFromCommitMetadata(HoodieCommitMetadata
commitMetadata) {
Review Comment:
🤖 nit: the method returns a `List` of paths (plural) but the name says
`getPartitionPath` — could you rename it to
`getPartitionPathsFromCommitMetadata` for consistency with the return type?
<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]
