hudi-agent commented on code in PR #18988:
URL: https://github.com/apache/hudi/pull/18988#discussion_r3663424156
##########
hudi-client/hudi-flink-client/src/main/java/org/apache/hudi/client/HoodieFlinkTableServiceClient.java:
##########
@@ -100,6 +100,9 @@ protected void completeCompaction(HoodieCommitMetadata
metadata, HoodieTable tab
}
}
log.info("Compacted successfully on commit {}", compactionCommitTime);
+ log.info("Compacted successfully on commit " + compactionCommitTime);
Review Comment:
🤖 nit: this line duplicates the `log.info("Compacted successfully on commit
{}", ...)` already on line 102, and switches to string concatenation — could
you remove it?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-flink-client/src/main/java/org/apache/hudi/client/HoodieFlinkTableServiceClient.java:
##########
@@ -160,6 +163,9 @@ protected void completeClustering(
}
}
log.info("Clustering successfully on commit {}", clusteringCommitTime);
+ log.info("Clustering successfully on commit " + clusteringCommitTime);
Review Comment:
🤖 nit: same as above — this duplicates the clustering log on line 165 and
uses string concatenation; could you drop this line?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -287,7 +283,7 @@ public boolean commitStats(String instantTime,
TableWriteStats tableWriteStats,
boolean postCommitStatus = true;
HoodieTimer postCommitTimer = HoodieTimer.start();
try {
- postCommit(table, metadata, instantTime, extraMetadata);
+ postCommit(table, metadata, instantTime, commitActionType,
extraMetadata);
Review Comment:
🤖 Confirmed this against the current head: the callback now fires inside
`postCommit` (line 672), which `commitStats()` invokes *before*
`mayBeCleanAndArchive`/`runTableServicesInline` (lines 288-290). Pre-PR the
fire happened only after those steps returned, so with
`canIgnorePostCommitFailures=false` a consumer could now receive a success
callback for a commit whose `commitStats()` subsequently throws from
clean/archival. The underlying commit is already durable at that point, so this
may be acceptable, but it is a real change in ordering semantics that the
description doesn't call out — worth confirming it's intentional (or moving the
data-path fire back after `runTableServicesInline`).
--
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]
