rahil-c commented on code in PR #18942:
URL: https://github.com/apache/hudi/pull/18942#discussion_r3814654248
##########
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:
Done, reverted to `CLUSTERING_ACTION`.
Your read of the convention is the one I ended up on: metrics in this class
are named after the table service, not the completed timeline action.
Compaction reports under `compaction.*` though it completes as `commit`, and
log compaction under `logcompaction.*` though it completes as `deltacommit`, so
`clustering.*` is what matches its neighbours. `replacecommit.*` would also
share gauges with insert_overwrite, delete_partition and bucket_rescale under
last-write-wins registration in `Metrics.registerGauge`.
That makes master already correct here, so rather than pick a constant I
have repointed this PR at the pending-instant bug you flagged in the review
body. Details in the top-level comment.
##########
hudi-client/hudi-flink-client/src/main/java/org/apache/hudi/client/HoodieFlinkTableServiceClient.java:
##########
@@ -155,7 +155,7 @@ protected void completeClustering(
long durationInMs = metrics.getDurationInMs(clusteringTimer.stop());
try {
metrics.updateCommitMetrics(TimelineUtils.parseDateFromInstantTime(clusteringCommitTime).getTime(),
- durationInMs, metadata, HoodieActiveTimeline.CLUSTERING_ACTION);
+ durationInMs, metadata, clusteringInstant.getAction());
Review Comment:
Done, reverted alongside the base client, so the whole family stays under
`clustering.*`. `HoodieMetrics:166` (`clustering.timer`), `:456`
(`clustering.fileCreationTime`) and `FlinkClusteringMetrics` were already on
that namespace and are untouched, so the Flink double-reporting you described
does not arise.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/metrics/TestHoodieMetrics.java:
##########
@@ -638,6 +638,41 @@ public void testGetMetricsNameWithoutPrefix() {
// Existing rollback-failure and conflict-resolution-by-category tests
// -----------------------------------------------------------------------
+ @Test
+ public void testClusteringCommitMetricsUsesVersionAwareAction() {
Review Comment:
Done, deleted.
The replacement goes through `updateTableServiceInstantMetrics` rather than
calling `updateCommitMetrics` with a literal. Verified it fails on master with
`expected: <1> but was: <0>`, and also fails against the naive version of the
fix (matching `replacecommit` by action name) with `expected: <1> but was:
<2>`, since the fixture puts an earlier pending insert_overwrite next to the
pending clustering instant.
--
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]
