voonhous commented on code in PR #18942:
URL: https://github.com/apache/hudi/pull/18942#discussion_r3803022111
##########
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:
This test passes unchanged if both production hunks are reverted, so it
cannot catch a regression in what this PR changes.
It calls `hoodieMetrics.updateCommitMetrics(...)` directly with a string
literal and never reaches `BaseHoodieTableServiceClient.completeClustering` or
`HoodieFlinkTableServiceClient.completeClustering`:
```
grep -n "TableServiceClient\|completeClustering"
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/metrics/TestHoodieMetrics.java
```
returns nothing. `updateCommitMetrics` never branches on the action and
`getMetricsName` is just `String.format("%s.%s", action, metric)`
(`HoodieMetrics.java:444-453`), so passing `CLUSTERING_ACTION` and then
asserting the gauge landed at `getMetricsName(CLUSTERING_ACTION, ...)` is a
tautology. The `assertNull` on the replacecommit name is true by construction
as well, since `@BeforeEach` builds a fresh registry per test and nothing
pre-registers it. Codecov agrees: 0% patch coverage, with both changed
production lines reported "Missing".
It is also a near-duplicate of `testTimerCtxandGauges`
(`TestHoodieMetrics.java:186`), which already runs this same assertion
parameterised over `Stream.of("commit", "deltacommit", "compaction")`.
Action: delete `testClusteringCommitMetricsUsesVersionAwareAction` and put
the assertion where it runs the changed line --
`TestHoodieFlinkTableServiceClient.testCompleteClusteringCommitsAndCleansMarkers`
(`hudi-client/hudi-flink-client/src/test/java/org/apache/hudi/client/TestHoodieFlinkTableServiceClient.java:225`).
#19464 already built that harness and stubs
`when(clusteringInstant.getAction()).thenReturn(...)`, so parameterising it
over `{CLUSTERING_ACTION, REPLACE_COMMIT_ACTION}`, turning metrics on and
asserting the registered gauge name is the chosen constant in both cases is a
handful of lines, and it would fail on the current code. If you also want cheap
coverage of the naming path itself, add `"clustering"` and `"replacecommit"` to
the existing `Stream.of(...)` at line 186 rather than a new 35-line method.
##########
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:
Whichever constant is settled on in the thread on
`BaseHoodieTableServiceClient.java:640`, it has to be applied to the rest of
the family or one clustering commit ends up split across two metric namespaces.
On a table-version-6 table `clusteringInstant.getAction()` is
`replacecommit` -- `ClusteringPlanActionExecutor.java:93-94` schedules v6
clustering as `REPLACE_COMMIT_ACTION` ("To support writing and reading with
table version SIX, we need to allow instant action to be
REPLACE_COMMIT_ACTION"). These two stay hardcoded to `clustering` regardless:
- `HoodieMetrics.java:166` -> `clustering.timer`
- `HoodieMetrics.java:456` -> `clustering.fileCreationTime`, called from
`BaseHoodieTableServiceClient.java:595` in the same clustering flow
So a single v6 clustering commit would emit `clustering.timer` +
`clustering.fileCreationTime` + `replacecommit.duration` +
`replacecommit.totalRecordsWritten`.
The Flink streaming path is worse, because it double-reports.
`ClusteringCommitSink.java:219` calls
`completeTableService(TableServiceType.CLUSTER, ...)`, which reaches this line,
and then `:221` calls `clusteringMetrics.updateCommitMetrics(...)`.
`FlinkClusteringMetrics` hardcodes its action to `"clustering"`
(`FlinkClusteringMetrics.java:44,69`) and `FlinkWriteMetrics.registerMetrics`
publishes the same gauge family this call does -- `totalPartitionsWritten`,
`totalRecordsWritten`, `totalBytesWritten`, `commitTime`, `duration`
(`FlinkWriteMetrics.java:57-71`). On a v6 table one Flink job would therefore
report the same commit under both `clustering.*` and `replacecommit.*`.
Action: apply the chosen constant to `HoodieMetrics.java:166`,
`HoodieMetrics.java:456` and `FlinkClusteringMetrics.java:44` in this PR as
well, so the whole family stays in one namespace.
--
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]
