voonhous commented on code in PR #18942:
URL: https://github.com/apache/hudi/pull/18942#discussion_r3803017734
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -633,7 +633,7 @@ private void completeClustering(HoodieReplaceCommitMetadata
replaceCommitMetadat
if (clusteringTimer != null) {
long durationInMs = metrics.getDurationInMs(clusteringTimer.stop());
TimelineUtils.parseDateFromInstantTimeSafely(clusteringCommitTime).ifPresent(parsedInstant
->
- metrics.updateCommitMetrics(parsedInstant.getTime(), durationInMs,
replaceCommitMetadata, HoodieActiveTimeline.CLUSTERING_ACTION)
+ metrics.updateCommitMetrics(parsedInstant.getTime(), durationInMs,
replaceCommitMetadata, clusteringInstant.getAction())
Review Comment:
Confirming this, with the evidence, plus one caveat on which constant.
`clusteringInstant` here is the **inflight** instant
(`ClusteringUtils.getInflightClusteringInstant`, line 609), captured before the
transition. The completed instant is always `replacecommit`:
`ActiveTimelineV2.transitionClusterInflightToComplete` builds it with
`REPLACE_COMMIT_ACTION` even when the inflight action was `clustering`
(`ActiveTimelineV2.java:513-518`), and `ActiveTimelineV1` delegates straight to
the replace path (`ActiveTimelineV1.java:497-500`).
That is the same reasoning that removed this exact expression from line 647,
seven lines below: `79fedcf12f43` ("fix(client): report completed timeline
action in clustering callback", #19464, merged 2026-08-06). `git merge-base
--is-ancestor 79fedcf12f43 HEAD` is true on this branch, so lines 640 and 647
currently use opposite conventions for the same event.
Caveat before settling on `replacecommit`: `6456d7796006` ([HUDI-7905],
#11553) deliberately changed this very line `REPLACE_COMMIT_ACTION ->
CLUSTERING_ACTION`, with the rationale "Currently, we use replacecommit for
clustering, insert overwrite and delete partition. Clustering should be a
separate action." `INSERT_OVERWRITE`, `INSERT_OVERWRITE_TABLE`,
`DELETE_PARTITION` and `BUCKET_RESCALE` all report under `replacecommit`
(`CommitUtils.java:60-66`), and `Metrics.registerGauge` is last-write-wins on a
duplicate name (`Metrics.java:161-170`), so `replacecommit.totalRecordsWritten`
would mix clustering with insert-overwrite. Worth noting too that metrics in
this class key off the service name while callbacks key off the completed
action: line 423 emits `compaction` (completed instant is `commit`), line 497
emits `logcompaction` (completed is `deltacommit`), while 427/501/647 fire
`commit`/`deltacommit`/`replacecommit`.
Concretely: replace `clusteringInstant.getAction()` with a constant here and
at `HoodieFlinkTableServiceClient.java:158`, and state in the PR description
which convention you are applying. If `replacecommit`, then
`HoodieMetrics.java:166` (`clustering.timer`) and `:456`
(`clustering.fileCreationTime`) have to move with it, and it is a rename of
metrics shipped since release-1.0.0 so it needs a release note. If
`clustering`, master is already correct and this PR is a no-op.
If you take `replacecommit`:
```suggestion
metrics.updateCommitMetrics(parsedInstant.getTime(), durationInMs,
replaceCommitMetadata, HoodieTimeline.REPLACE_COMMIT_ACTION)
```
--
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]
