rahil-c commented on PR #18942: URL: https://github.com/apache/hudi/pull/18942#issuecomment-5344739661
@voonhous thanks for the depth on this one, the archaeology on #11553 and #19464 is what settled it. I have repointed the PR rather than pick a constant. Summary of where it landed: **The naming question.** Master is already right. Metrics in `BaseHoodieTableServiceClient` are named after the table service, not the completed timeline action: `:423` reports `compaction.*` while the instant completes as `commit`, and `:497` reports `logcompaction.*` while it completes as `deltacommit`. `clustering.*` is consistent with both. `replacecommit.*` would additionally collide with insert_overwrite, insert_overwrite_table, delete_partition and bucket_rescale on shared gauges, since `Metrics.registerGauge` reuses the gauge and calls `setValue`. So both production hunks are reverted and no metric is renamed. @nsivabalan, flagging that this walks back your request on line 640; the reasoning is above and I am happy to revisit if you see it differently. **What the PR does now** is the version-aware defect from your review body, which is what the title promised all along. `HoodieMetrics.updateTableServiceInstantMetrics` selects pending clustering by exact action name, so on table version six `clustering.pendingClusteringInstantCount` sits at 0 forever and `clustering.earliestPendingClusteringInstant` is never registered. The completed counterpart already uses `REPLACE_COMMIT_ACTION` and is fine, so only the pending half was version-blind. Selection now goes through `ClusteringUtils.isClusteringInstant`, which short-circuits on `CLUSTERING_ACTION` so layout 2 stays a string comparison, and only reads requested metadata for a pending `replacecommit`. Read failures are caught and logged so metrics cannot fail a commit. The action string still names the metric, so the names are unchanged on every table version. **Verification.** `TestHoodieMetrics` is green at 24 tests. The new test fails on master with `expected: <1> but was: <0>`, and fails against the naive fix of adding `REPLACE_COMMIT_ACTION` to the filter with `expected: <1> but was: <2>`, because the fixture places an earlier pending insert_overwrite beside the pending clustering instant. Description is filled in, so `validate-pr` should go green on the next run. -- 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]
