rahil-c commented on code in PR #18942:
URL: https://github.com/apache/hudi/pull/18942#discussion_r3858159963
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metrics/HoodieMetrics.java:
##########
@@ -456,8 +459,12 @@ public void updateClusteringFileCreationMetrics(long
durationInMs) {
reportMetrics(HoodieTimeline.CLUSTERING_ACTION, "fileCreationTime",
durationInMs);
}
- public void updateTableServiceInstantMetrics(final HoodieActiveTimeline
activeTimeline) {
- updateEarliestPendingInstant(activeTimeline,
EARLIEST_PENDING_CLUSTERING_INSTANT_STR, HoodieTimeline.CLUSTERING_ACTION);
+ public void updateTableServiceInstantMetrics(final HoodieActiveTimeline
activeTimeline, final InstantGenerator instantGenerator) {
+ // Clustering is scheduled as CLUSTERING_ACTION only on timeline layout 2.
On table version six it is scheduled as
+ // REPLACE_COMMIT_ACTION, which insert_overwrite and delete_partition
share, so the clustering plan is what
+ // identifies it rather than the action name.
Review Comment:
Moot now, that block is deleted. The javadoc on the replacement names the
requested replace metadata `operationType` rather than the plan, and the PR
body is corrected to match.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/metrics/TestHoodieMetrics.java:
##########
@@ -320,13 +324,61 @@ public void testTimerCtxandGauges() throws
InterruptedException {
assertEquals((long)metrics.getRegistry().getGauges().get(metricName).getValue(),
6L);
}
+ @Test
+ void testPendingClusteringInstantMetricsOnTableVersionSix() {
+ // Table version six schedules clustering as REPLACE_COMMIT_ACTION, which
insert_overwrite shares.
+ HoodieInstant pendingInsertOverwrite =
+ INSTANT_GENERATOR.createNewInstant(HoodieInstant.State.REQUESTED,
HoodieTimeline.REPLACE_COMMIT_ACTION, "1001");
+ HoodieInstant pendingClustering =
+ INSTANT_GENERATOR.createNewInstant(HoodieInstant.State.REQUESTED,
HoodieTimeline.REPLACE_COMMIT_ACTION, "1002");
+ Map<String, String> operationTypes = new HashMap<>();
+ operationTypes.put("1001", WriteOperationType.INSERT_OVERWRITE.name());
+ operationTypes.put("1002", WriteOperationType.CLUSTER.name());
+
+ hoodieMetrics.updateTableServiceInstantMetrics(
+ new MockClusteringPlanTimeline(operationTypes, pendingInsertOverwrite,
pendingClustering), INSTANT_GENERATOR);
+
+ // Only the instant carrying a clustering plan counts. insert_overwrite is
the earlier of the two, so
+ // both assertions fail if the action name alone is used to select it.
Review Comment:
Moot now, this test is deleted. Its replacement asserts `assertNotNull` on
each gauge before dereferencing, so a regression fails with the metric name
rather than an NPE.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/metrics/TestHoodieMetrics.java:
##########
@@ -320,13 +324,61 @@ public void testTimerCtxandGauges() throws
InterruptedException {
assertEquals((long)metrics.getRegistry().getGauges().get(metricName).getValue(),
6L);
}
+ @Test
+ void testPendingClusteringInstantMetricsOnTableVersionSix() {
+ // Table version six schedules clustering as REPLACE_COMMIT_ACTION, which
insert_overwrite shares.
+ HoodieInstant pendingInsertOverwrite =
+ INSTANT_GENERATOR.createNewInstant(HoodieInstant.State.REQUESTED,
HoodieTimeline.REPLACE_COMMIT_ACTION, "1001");
+ HoodieInstant pendingClustering =
+ INSTANT_GENERATOR.createNewInstant(HoodieInstant.State.REQUESTED,
HoodieTimeline.REPLACE_COMMIT_ACTION, "1002");
+ Map<String, String> operationTypes = new HashMap<>();
+ operationTypes.put("1001", WriteOperationType.INSERT_OVERWRITE.name());
+ operationTypes.put("1002", WriteOperationType.CLUSTER.name());
+
+ hoodieMetrics.updateTableServiceInstantMetrics(
+ new MockClusteringPlanTimeline(operationTypes, pendingInsertOverwrite,
pendingClustering), INSTANT_GENERATOR);
+
+ // Only the instant carrying a clustering plan counts. insert_overwrite is
the earlier of the two, so
+ // both assertions fail if the action name alone is used to select it.
+ String countMetric =
hoodieMetrics.getMetricsName(HoodieTimeline.CLUSTERING_ACTION,
HoodieMetrics.PENDING_CLUSTERING_INSTANT_COUNT_STR);
+ assertEquals(1L, (long)
metrics.getRegistry().getGauges().get(countMetric).getValue());
+ String earliestMetric =
hoodieMetrics.getMetricsName(HoodieTimeline.CLUSTERING_ACTION,
HoodieMetrics.EARLIEST_PENDING_CLUSTERING_INSTANT_STR);
+ assertEquals(1002L, (long)
metrics.getRegistry().getGauges().get(earliestMetric).getValue());
+ }
+
private static class MockHoodieActiveTimeline extends ActiveTimelineV2 {
public MockHoodieActiveTimeline(HoodieInstant... instants) {
super();
this.setInstants(Arrays.asList(instants));
}
}
+ /**
+ * Serves a requested replace metadata per instant time so pending
replacecommits can be told apart
+ * by their write operation type without a backing timeline on storage.
+ */
+ private static class MockClusteringPlanTimeline extends
MockHoodieActiveTimeline {
Review Comment:
Moot now, `MockClusteringPlanTimeline` is deleted. The new test calls the
`TestBaseHoodieTableServiceClient` helper directly, with a
`transitionToInflight` flag added so it can produce both pending states, so
there is no third copy of the builder chain.
--
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]
