codope commented on code in PR #19464:
URL: https://github.com/apache/hudi/pull/19464#discussion_r3702901647
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -644,7 +645,7 @@ private void completeClustering(HoodieReplaceCommitMetadata
replaceCommitMetadat
heartbeatClient.stop(clusteringCommitTime);
}
log.info("Clustering successfully on commit {} for table {}",
clusteringCommitTime, table.getConfig().getBasePath());
- fireCommitCallbackIfNecessary(clusteringCommitTime,
clusteringInstant.getAction(),
+ fireCommitCallbackIfNecessary(clusteringCommitTime,
completedClusteringInstant.getAction(),
Review Comment:
I don't think we can. The `CLUSTERING_ACTION` is used to build the metric
key name (`getMetricsName(actionType, ...)` etc.). So, it's a label for the
metric namespace, not necessarily about what's on the timeline. The callback's
`commitActionType` is different: its whole contract (and the reason this PR and
#18988 exist) is to report the action the commit actually completed under, so
downstream consumers can correctly interpret the commit. On table version 8+ /
layout v2, the completed instant is replacecommit even though the inflight one
is clustering (that's the bug). If we hardcode `CLUSTERING_ACTION`, we'd
reintroduce the issue and report clustering for a commit whose timeline entry
is actually replacecommit.
I did think of hardcode `REPLACE_COMMIT_ACTION` at the two call sites, but
that duplicates the completed-action mapping across ActiveTimelineV1/V2 and
both call sites, and would go stale silently if that mapping ever changes.
Returning the completed instant from
`transitionClusteringOrReplaceInflightToComplete` keeps that mapping defined in
exactly one place which is reusable, so I'd prefer to keep it.
--
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]
