nsivabalan commented on code in PR #18279:
URL: https://github.com/apache/hudi/pull/18279#discussion_r2898314155
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/RollbackHelperV1.java:
##########
@@ -216,14 +219,25 @@ List<Pair<String, HoodieRollbackStat>>
maybeDeleteAndCollectStats(HoodieEngineCo
.withStorage(metaClient.getStorage())
.withFileCreationCallback(getRollbackLogMarkerCallback(writeMarkers,
partitionPath, fileId))
.withTableVersion(tableVersion)
- .withFileExtension(HoodieLogFile.DELTA_EXTENSION).build();
+ .withFileExtension(HoodieLogFile.DELTA_EXTENSION);
+
+ String logVersionKey = logVersionLookupKey(partitionPath, fileId,
rollbackRequest.getLatestBaseInstant());
Review Comment:
Apply my feedback just for RollbackHelperV1 only.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/RollbackHelper.java:
##########
@@ -169,13 +185,17 @@ List<Pair<String, HoodieRollbackStat>>
maybeDeleteAndCollectStats(HoodieEngineCo
}
}
- // This step is intentionally done after writer is closed. Guarantees
that
- // getFileStatus would reflect correct stats and FileNotFoundException
is not thrown in
- // cloud-storage : HUDI-168
- Map<StoragePathInfo, Long> filesToNumBlocksRollback =
Collections.singletonMap(
-
metaClient.getStorage().getPathInfo(Objects.requireNonNull(filePath)),
- 1L
- );
+ Map<StoragePathInfo, Long> filesToNumBlocksRollback;
+ if (fileSizeCaptured) {
+ filesToNumBlocksRollback = Collections.singletonMap(
+ new StoragePathInfo(Objects.requireNonNull(filePath), fileSize,
false, (short) 0, 0, 0), 1L);
+ } else {
+ // This step is intentionally done after writer is closed.
Guarantees that
+ // getFileStatus would reflect correct stats and
FileNotFoundException is not thrown in
Review Comment:
this comment contradicts w/ the optimization in L171 right?
so, likely this might be applicable only for hdfs?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/RollbackHelper.java:
##########
@@ -146,14 +151,25 @@ List<Pair<String, HoodieRollbackStat>>
maybeDeleteAndCollectStats(HoodieEngineCo
)
.withStorage(metaClient.getStorage())
.withTableVersion(tableVersion)
- .withFileExtension(HoodieLogFile.DELTA_EXTENSION).build();
+ .withFileExtension(HoodieLogFile.DELTA_EXTENSION);
+
+ String logVersionKey =
logVersionLookupKey(rollbackRequest.getPartitionPath(), fileId,
rollbackRequest.getLatestBaseInstant());
+ Pair<Integer, String> preComputedVersion =
logVersionMap.get(logVersionKey);
+ if (preComputedVersion != null) {
+ writerBuilder.withLogVersion(preComputedVersion.getLeft())
+ .withLogWriteToken(preComputedVersion.getRight());
+ }
Review Comment:
if not present, should we set it to `UNKNOWN_WRITE_TOKEN`
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/RollbackHelper.java:
##########
@@ -146,14 +151,25 @@ List<Pair<String, HoodieRollbackStat>>
maybeDeleteAndCollectStats(HoodieEngineCo
)
.withStorage(metaClient.getStorage())
.withTableVersion(tableVersion)
- .withFileExtension(HoodieLogFile.DELTA_EXTENSION).build();
+ .withFileExtension(HoodieLogFile.DELTA_EXTENSION);
+
Review Comment:
Can you run tests w/ v9 and confirm this code blocks are touched?
from what I know, in v9, we don't add any log blocks for rollbacks.
Looks like we just left the code from old classes as is. but in reality for
v9, we should not reach here?
i.e. `getLogBlocksToBeDeleted()` should be empty
--
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]
