rahil-c commented on code in PR #18942:
URL: https://github.com/apache/hudi/pull/18942#discussion_r3858158376
##########
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.
+ Predicate<HoodieInstant> pendingClustering = instant ->
isPendingClusteringInstant(activeTimeline, instant, instantGenerator);
+ updateEarliestPendingInstant(activeTimeline,
EARLIEST_PENDING_CLUSTERING_INSTANT_STR, HoodieTimeline.CLUSTERING_ACTION,
pendingClustering);
Review Comment:
Done. `updatePendingClusteringInstantMetrics` calls
`activeTimeline.filterPendingClusteringTimeline()` once and feeds both
clustering metrics from it. The `Predicate` overloads, the `InstantGenerator`
parameter and `isPendingClusteringInstant` are gone, both helpers are back to
their master form, and `BaseHoodieWriteClient` is byte-identical to master so
it leaves the PR entirely. The production change is now 29 lines in one file.
The read-count assertion in the new test pins the single call per commit.
##########
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) {
Review Comment:
Done, guard added and `updateTableServiceInstantMetrics` is now in the
`testUpdateMethodsAreNoOpsWhenMetricsOff` inventory.
Worth flagging that the inventory test passes with or without the guard,
since it only proves no NPE. To actually pin the elision I added
`testTableServiceInstantMetricsReadPendingClusteringOncePerCommitAndNeverWhenMetricsOff`,
which counts `filterPendingClusteringTimeline` calls: 0 with metrics off, 1
with metrics on. Removing the guard fails it with `expected: <0> but was: <1>`,
and the second assertion also pins the single read you raised above.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metrics/HoodieMetrics.java:
##########
@@ -484,13 +491,35 @@ private void updateEarliestPendingInstant(final
HoodieActiveTimeline activeTimel
final String metricName,
final String action) {
Set<String> validActions = CollectionUtils.createSet(action);
- HoodieTimeline filteredInstants =
activeTimeline.filterInflightsAndRequested().filter(instant ->
validActions.contains(instant.getAction()));
+ updateEarliestPendingInstant(activeTimeline, metricName, action, instant
-> validActions.contains(instant.getAction()));
+ }
+
+ private void updateEarliestPendingInstant(final HoodieActiveTimeline
activeTimeline,
+ final String metricName,
+ final String action,
+ final Predicate<HoodieInstant>
pendingFilter) {
+ HoodieTimeline filteredInstants =
activeTimeline.filterInflightsAndRequested().filter(pendingFilter);
Option<HoodieInstant> hoodieInstantOption =
filteredInstants.firstInstant();
if (hoodieInstantOption.isPresent()) {
updateTimestampMetric(metricName, action, hoodieInstantOption);
}
}
+ /**
+ * Whether a pending instant belongs to a clustering operation, on any table
version.
+ * A plan that cannot be read is treated as non-clustering so that metrics
never fail the commit.
+ */
+ private boolean isPendingClusteringInstant(final HoodieActiveTimeline
activeTimeline,
+ final HoodieInstant instant,
+ final InstantGenerator
instantGenerator) {
+ try {
+ return ClusteringUtils.isClusteringInstant(activeTimeline, instant,
instantGenerator);
+ } catch (Exception e) {
+ log.warn("Failed to read the replace metadata of {} while updating
clustering instant metrics", instant, e);
+ return false;
+ }
Review Comment:
Done on both. It logs at `debug` now, and
`testPendingClusteringInstantMetricsSurviveUnreadableReplaceMetadata` covers
the fallback: a timeline whose `filterPendingClusteringTimeline` throws must
not propagate, and the count gauge must still register 0. Dropping the catch
fails it with the propagated `HoodieIOException`.
##########
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() {
Review Comment:
Done, rebuilt on that harness.
`updateTableServiceInstantMetrics_reportsPendingClusteringPerTableVersion` in
`TestBaseHoodieTableServiceClient` is parameterised on `preTableVersion8`, so
the true leg runs a real `HoodieTableVersion.SIX` metaClient on layout 1. It
fails on master with `expected: <2> but was: <0>` on that leg only, while the
layout 2 leg still passes, so it isolates the version-specific bug rather than
asserting a tautology. The layout 2 test in `TestHoodieMetrics` is deleted.
##########
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());
Review Comment:
Done on both. The fixture now holds a REQUESTED and an INFLIGHT clustering
instant and asserts a count of 2, and the insert_overwrite is written with
`createRequestedCommitWithReplaceMetadata`, so its `operationType` is genuinely
null rather than the string. Adding `REPLACE_COMMIT_ACTION` to the action
filter now fails both legs with `expected: <2> but was: <3>`.
--
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]
