hudi-agent commented on code in PR #19661:
URL: https://github.com/apache/hudi/pull/19661#discussion_r4235858629
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -581,6 +581,9 @@ private void commitClustering(HoodieWriteMetadata<O>
clusteringWriteMetadata, Ho
replaceCommitMetadata.addWriteStat(writeStat.getPartitionPath(),
writeStat);
}
HoodieClusteringPlan clusteringPlan =
ClusteringUtils.getPendingClusteringPlan(table.getMetaClient(),
clusteringCommitTime);
+ if (clusteringPlan.getExtraMetadata() != null) {
+
clusteringPlan.getExtraMetadata().forEach(replaceCommitMetadata::addMetadata);
Review Comment:
🤖 non-blocking: It looks like the scheduling-time `hudi.version`/`engine`
end up on `HoodieRequestedReplaceMetadata.extraMetadata`
(`ClusteringPlanActionExecutor` line ~99), not on
`HoodieClusteringPlan.extraMetadata`. The plan's own map only holds strategy
keys such as `clustering.commit.checkpoint`, so completed clustering commits
still won't carry the debug keys. Would it make sense to enrich in
`completeClustering` instead? That path also covers Flink's
`ClusteringCommitSink`, which skips `commitClustering`:
```java
updateExtraMetadata(Option.empty()).get().forEach(replaceCommitMetadata::addMetadata);
```
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/table/TestBaseHoodieTable.java:
##########
@@ -130,7 +130,8 @@ public HoodieCleanMetadata clean(HoodieEngineContext
context, String cleanInstan
@Override
public Option<HoodieRollbackPlan> scheduleRollback(HoodieEngineContext
context, String instantTime, HoodieInstant instantToRollback,
- boolean
skipTimelinePublish, boolean shouldRollbackUsingMarkers, boolean isRestore) {
+ boolean
skipTimelinePublish, boolean shouldRollbackUsingMarkers, boolean isRestore,
+ Option extraMetadata) {
Review Comment:
🤖 nit: could you use `Option<Map<String, String>>` instead of the raw
`Option` here (and in `scheduleRestore`)? It matches the abstract signature and
avoids raw-type warnings.
<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/RestorePlanActionExecutor.java:
##########
@@ -100,7 +104,7 @@ public Option<HoodieRestorePlan> execute() {
.map(entry -> new HoodieInstantInfo(entry.requestedTime(),
entry.getAction()))
.collect(Collectors.toList());
}
- HoodieRestorePlan restorePlan = new
HoodieRestorePlan(instantsToRollback, LATEST_RESTORE_PLAN_VERSION,
savepointToRestoreTimestamp);
+ HoodieRestorePlan restorePlan = new
HoodieRestorePlan(instantsToRollback, LATEST_RESTORE_PLAN_VERSION,
savepointToRestoreTimestamp, extraMetadata.orElse(null));
Review Comment:
🤖 nit: this line is now quite long; could you wrap the constructor arguments?
<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]
