voonhous commented on code in PR #19868:
URL: https://github.com/apache/hudi/pull/19868#discussion_r3961457864
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -1543,8 +1543,20 @@ public void performTableServices(Option<String>
inFlightInstantTimestamp, boolea
.filterCompletedInstants()
.lastInstant();
if (!lastInstant.isPresent()) {
+ metrics.ifPresent(m -> m.updateDeltaCommitsSinceLastCompaction(0L));
return;
}
+ // Report how far the metadata table has drifted from its last
compaction, so that a stalled
+ // compaction is visible before the log to base file ratio degrades read
performance. This is the
+ // backlog as it stands before the compaction below runs, so on a
healthy table it peaks at
+ // hoodie.metadata.compact.max.delta.commits on the cycle that compacts
and drops on the next one.
+ // It counts the same completed instants that
ScheduleCompactionActionExecutor compares against
+ // that threshold, and is not re-sampled afterwards because doing so
would need another timeline
+ // reload on the write path.
+ metrics.ifPresent(m -> m.updateDeltaCommitsSinceLastCompaction(
Review Comment:
**major:** This sample sits after
`runPendingTableServicesOperationsAndRefreshTimeline`, which rethrows (L1623)
when a pending MDT compaction fails, so in the "compaction keeps failing" state
this line is never reached again and the gauge freezes at its last value while
delta commits keep landing. That is silent only with
`hoodie.metadata.write.fail.on.table.service.failures=false`, but that is the
deployment relying on a metric. Could we sample in the `finally` block next to
`TABLE_SERVICE_EXECUTION_DURATION`, from
`metadataMetaClient.getActiveTimeline()`, so the failure path reports too?
<details><summary>trace</summary>
`compactIfNecessary` -> `writeClient.compact` throws -> L1667
`COMPACTION_FAILURES++`, rethrow -> compaction instant left pending. Next
cycle: L1539 -> `filterPendingCompactionTimeline().countInstants() > 0` ->
`runAnyPendingCompactions()` throws -> L1621 `PENDING_COMPACTIONS_FAILURES++`,
L1623 rethrow -> outer catch at L1570 -> L1556 skipped. Repeats every cycle
until the pending compaction succeeds.
</details>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -1543,8 +1543,20 @@ public void performTableServices(Option<String>
inFlightInstantTimestamp, boolea
.filterCompletedInstants()
.lastInstant();
if (!lastInstant.isPresent()) {
+ metrics.ifPresent(m -> m.updateDeltaCommitsSinceLastCompaction(0L));
return;
}
+ // Report how far the metadata table has drifted from its last
compaction, so that a stalled
+ // compaction is visible before the log to base file ratio degrades read
performance. This is the
+ // backlog as it stands before the compaction below runs, so on a
healthy table it peaks at
+ // hoodie.metadata.compact.max.delta.commits on the cycle that compacts
and drops on the next one.
+ // It counts the same completed instants that
ScheduleCompactionActionExecutor compares against
+ // that threshold, and is not re-sampled afterwards because doing so
would need another timeline
+ // reload on the write path.
+ metrics.ifPresent(m -> m.updateDeltaCommitsSinceLastCompaction(
+
CompactionUtils.getCompletedDeltaCommitsSinceLatestCompaction(activeTimeline)
+ .map(deltaCommitsInfo -> (long)
deltaCommitsInfo.getLeft().countInstants())
+ .orElse(0L)));
Review Comment:
**nit:** Feel free to ignore. `.orElse(0L)` is unreachable: L1544 already
returned unless a completed delta commit exists, and
`getDeltaCommitsSinceLatestCompaction` returns empty only when the delta-commit
timeline is empty (`CompactionUtils.java:329-335`). Could we use `.get()` here,
or keep the fallback with a one-line "defensive" comment?
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataMetrics.java:
##########
@@ -84,6 +85,22 @@ public class HoodieMetadataMetrics implements Serializable {
public static final String LOG_COMPACTION_FAILURES =
"logcompaction_failures";
public static final String PENDING_COMPACTIONS_FAILURES =
"pending_compactions_failures";
+ // Metadata table compaction health. The existing per-partition
baseFileCount/logFileCount gauges
+ // show the current shape of the metadata table, but not whether compaction
is keeping up with it.
+ // Completed delta commits on the metadata table since the last completed
compaction, sampled before
+ // table services run. On a healthy table this sawtooths up to
hoodie.metadata.compact.max.delta.commits
+ // and falls back after each compaction, so alert on a multiple of that
config rather than on the
+ // config value itself. A value that climbs past the peak and keeps going
means metadata table
+ // compaction is not being scheduled or is failing.
+ public static final String STAT_DELTA_COMMITS_SINCE_LAST_COMPACTION =
"deltaCommitsSinceLastCompaction";
Review Comment:
**major:** None of the four new names has a dot, so the CloudWatch reporter
drops them: `CloudWatchReporter.stageMetricDatum` (c63c9bfa79fd, #19476) skips
dot-less names, citing open #19507, which names `HoodieMetadataMetrics` as the
cause. HUDI-9068 (100e9ac47590) prefixed `TABLE_SERVICE_EXECUTION_*` with the
MDT table name in `performTableServices` for exactly this reason. Could we
prefix these four the same way (`writeClient.getConfig().getTableName()` there,
`metaClient.getTableConfig().getTableName()` in `updateSizeMetrics`)?
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataMetrics.java:
##########
@@ -164,6 +181,48 @@ public void updateSizeMetrics(HoodieTableMetaClient
metaClient, HoodieBackedTabl
for (Map.Entry<String, String> e : stats.entrySet()) {
setMetric(e.getKey(), Long.parseLong(e.getValue()));
}
+ long totalBaseFiles = sumStat(stats, metadataPartitions,
STAT_COUNT_BASE_FILES);
+ long totalLogFiles = sumStat(stats, metadataPartitions,
STAT_COUNT_LOG_FILES);
+ setMetric(STAT_TOTAL_BASE_FILE_COUNT, totalBaseFiles);
+ setMetric(STAT_TOTAL_LOG_FILE_COUNT, totalLogFiles);
+ setMetric(STAT_LOG_TO_BASE_FILE_RATIO_PERCENT,
logToBaseFileRatioPercent(totalBaseFiles, totalLogFiles));
Review Comment:
**major:** `updateSizeMetrics` has no test anywhere (`grep -rn
updateSizeMetrics` hits only two main-source lines), and the three new gauges
are asserted only as pure helpers, so swapping the two `setMetric` arguments on
L186-187 passes every test. Both existing `testMetadataMetrics`
(`TestHoodieBackedMetadata.java:3812-3815`,
`TestJavaHoodieBackedMetadata.java:2616-2623`) already assert the per-partition
gauges from this same call. Could we add `containsKey` for the four new names
there, rather than in a new file?
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataMetrics.java:
##########
@@ -164,6 +181,48 @@ public void updateSizeMetrics(HoodieTableMetaClient
metaClient, HoodieBackedTabl
for (Map.Entry<String, String> e : stats.entrySet()) {
setMetric(e.getKey(), Long.parseLong(e.getValue()));
}
+ long totalBaseFiles = sumStat(stats, metadataPartitions,
STAT_COUNT_BASE_FILES);
+ long totalLogFiles = sumStat(stats, metadataPartitions,
STAT_COUNT_LOG_FILES);
+ setMetric(STAT_TOTAL_BASE_FILE_COUNT, totalBaseFiles);
+ setMetric(STAT_TOTAL_LOG_FILE_COUNT, totalLogFiles);
+ setMetric(STAT_LOG_TO_BASE_FILE_RATIO_PERCENT,
logToBaseFileRatioPercent(totalBaseFiles, totalLogFiles));
+ }
+
+ /**
+ * Sums a per-partition stat, keyed as {@code <partition>.<statName>}, over
the enabled metadata partitions.
+ */
+ @VisibleForTesting
+ static long sumStat(Map<String, String> stats, Set<String>
metadataPartitions, String statName) {
+ return metadataPartitions.stream()
+ .mapToLong(partition -> Long.parseLong(stats.getOrDefault(partition +
"." + statName, "0")))
+ .sum();
+ }
+
+ /**
+ * Log files per base file expressed as a percentage, so that it can be
reported through a long valued gauge.
+ *
+ * <p>Returns 0 when there is no base file to divide by. That case is not
necessarily healthy: a metadata
+ * table holding log files but no base file has never been compacted. Read
this gauge together with
+ * {@link #STAT_TOTAL_BASE_FILE_COUNT} and {@link
#STAT_TOTAL_LOG_FILE_COUNT} rather than on its own.
+ */
+ @VisibleForTesting
+ static long logToBaseFileRatioPercent(long totalBaseFiles, long
totalLogFiles) {
+ return totalBaseFiles > 0 ? (totalLogFiles * 100L) / totalBaseFiles : 0L;
+ }
+
+ /**
+ * Reports the number of completed metadata table delta commits since the
last completed compaction.
+ *
+ * <p>This counts the same instants that {@code
ScheduleCompactionActionExecutor#needCompact} compares
+ * against the metadata table's delta commit threshold, which is configured
by
+ * {@code hoodie.metadata.compact.max.delta.commits} (default 10) under the
default {@code NUM_COMMITS}
+ * trigger strategy. Compaction fires once the count reaches that threshold,
and the gauge is sampled
+ * before table services run, so on a healthy table it peaks at the
threshold on the cycle where
+ * compaction fires and drops on the next one. Alert on a multiple of the
threshold; a stalled
Review Comment:
**major:** "climbs past it without bound" does not hold for a
never-scheduled stall.
`CompactionUtils.getEarliestInstantToRetainForCompaction` (L451-457) pins the
last compaction only while `numDeltaCommits < max`; at 10 or more the pin moves
to a delta commit, MDT archival (`keep.min+1 / keep.max+1` = 21/31,
`HoodieMetadataWriteUtils.java:293-296`) drops the compaction, and the helper's
"no compaction" branch saturates the gauge at 21-31, indistinguishable from
never-compacted. An alert at 3x-4x never fires. Could we reword this and the PR
body to say it saturates near `hoodie.keep.min.commits`, and alert at about 2x
the threshold?
<details><summary>why the other stall modes differ</summary>
A TSM-delegated stall leaves the compaction instant pending, which
`TimelineArchiverV2` candidate #1 (earliest pending instant) pins, so that one
does climb. A failing compaction never reaches the sample at all (see
`HoodieBackedTableMetadataWriter.java:1556`). Only the never-scheduled case
(e.g. `validateCompactionScheduling` false behind a long-running DT instant)
hits the plateau; MDT candidate #5 pins at the DT's earliest active instant,
which sits after the last MDT compaction, so it does not retain it either.
</details>
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataMetrics.java:
##########
@@ -164,6 +181,48 @@ public void updateSizeMetrics(HoodieTableMetaClient
metaClient, HoodieBackedTabl
for (Map.Entry<String, String> e : stats.entrySet()) {
setMetric(e.getKey(), Long.parseLong(e.getValue()));
}
+ long totalBaseFiles = sumStat(stats, metadataPartitions,
STAT_COUNT_BASE_FILES);
+ long totalLogFiles = sumStat(stats, metadataPartitions,
STAT_COUNT_LOG_FILES);
+ setMetric(STAT_TOTAL_BASE_FILE_COUNT, totalBaseFiles);
+ setMetric(STAT_TOTAL_LOG_FILE_COUNT, totalLogFiles);
+ setMetric(STAT_LOG_TO_BASE_FILE_RATIO_PERCENT,
logToBaseFileRatioPercent(totalBaseFiles, totalLogFiles));
+ }
+
+ /**
+ * Sums a per-partition stat, keyed as {@code <partition>.<statName>}, over
the enabled metadata partitions.
+ */
+ @VisibleForTesting
+ static long sumStat(Map<String, String> stats, Set<String>
metadataPartitions, String statName) {
+ return metadataPartitions.stream()
+ .mapToLong(partition -> Long.parseLong(stats.getOrDefault(partition +
"." + statName, "0")))
+ .sum();
+ }
+
+ /**
+ * Log files per base file expressed as a percentage, so that it can be
reported through a long valued gauge.
+ *
+ * <p>Returns 0 when there is no base file to divide by. That case is not
necessarily healthy: a metadata
+ * table holding log files but no base file has never been compacted. Read
this gauge together with
+ * {@link #STAT_TOTAL_BASE_FILE_COUNT} and {@link
#STAT_TOTAL_LOG_FILE_COUNT} rather than on its own.
+ */
+ @VisibleForTesting
+ static long logToBaseFileRatioPercent(long totalBaseFiles, long
totalLogFiles) {
+ return totalBaseFiles > 0 ? (totalLogFiles * 100L) / totalBaseFiles : 0L;
Review Comment:
**minor:** Not blocking. This ratio is file-group-weighted: `getStats`
counts per latest file slice, `files` has one file group and `record_index`
defaults to 10 (global: 10000), so RLI drowns the partition that actually hurts
listings. Worked case: `files` 1 base + 20 logs, RLI 1000 + 1000 reports 101%,
"about one log per base", while every partition listing merges 20 log files.
Since `getStats` already has the per-partition ints, could we emit the max
ratio across partitions instead of the sum ratio?
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataMetrics.java:
##########
@@ -164,6 +181,48 @@ public void updateSizeMetrics(HoodieTableMetaClient
metaClient, HoodieBackedTabl
for (Map.Entry<String, String> e : stats.entrySet()) {
setMetric(e.getKey(), Long.parseLong(e.getValue()));
}
+ long totalBaseFiles = sumStat(stats, metadataPartitions,
STAT_COUNT_BASE_FILES);
+ long totalLogFiles = sumStat(stats, metadataPartitions,
STAT_COUNT_LOG_FILES);
+ setMetric(STAT_TOTAL_BASE_FILE_COUNT, totalBaseFiles);
+ setMetric(STAT_TOTAL_LOG_FILE_COUNT, totalLogFiles);
+ setMetric(STAT_LOG_TO_BASE_FILE_RATIO_PERCENT,
logToBaseFileRatioPercent(totalBaseFiles, totalLogFiles));
+ }
+
+ /**
+ * Sums a per-partition stat, keyed as {@code <partition>.<statName>}, over
the enabled metadata partitions.
+ */
+ @VisibleForTesting
+ static long sumStat(Map<String, String> stats, Set<String>
metadataPartitions, String statName) {
+ return metadataPartitions.stream()
+ .mapToLong(partition -> Long.parseLong(stats.getOrDefault(partition +
"." + statName, "0")))
Review Comment:
**minor:** Not blocking. `getOrDefault(..., "0")` is unreachable: `getStats`
writes all four keys for every member of the same `metadataPartitions` set
(L132-158), so `sumStat` re-parses strings it just produced, and
`missingPartitionStatsAreTreatedAsZero` plus the `record_index` half of
`statsAreSummedAcrossEnabledPartitionsOnly` assert states production cannot
produce. Could we sum the two ints inside `getStats` and drop `sumStat` and
those two tests?
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataMetrics.java:
##########
@@ -84,6 +85,22 @@ public class HoodieMetadataMetrics implements Serializable {
public static final String LOG_COMPACTION_FAILURES =
"logcompaction_failures";
public static final String PENDING_COMPACTIONS_FAILURES =
"pending_compactions_failures";
+ // Metadata table compaction health. The existing per-partition
baseFileCount/logFileCount gauges
+ // show the current shape of the metadata table, but not whether compaction
is keeping up with it.
+ // Completed delta commits on the metadata table since the last completed
compaction, sampled before
+ // table services run. On a healthy table this sawtooths up to
hoodie.metadata.compact.max.delta.commits
+ // and falls back after each compaction, so alert on a multiple of that
config rather than on the
+ // config value itself. A value that climbs past the peak and keeps going
means metadata table
+ // compaction is not being scheduled or is failing.
Review Comment:
**minor:** Not blocking. With `hoodie.metadata.streaming.write.enabled`,
`performTableServices` is never called
(`FlinkHoodieBackedTableMetadataWriter.java:145` and
`HoodieFlinkTableServiceClient.java:209` both guard on
`!isStreamingWriteEnabled()`), so this gauge is never registered on the
deployment where MDT compaction runs in a separate pipeline and can stall
unnoticed. Could we note that gap in this comment, or emit it from the Flink
compaction pipeline as well?
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataMetrics.java:
##########
@@ -84,6 +85,22 @@ public class HoodieMetadataMetrics implements Serializable {
public static final String LOG_COMPACTION_FAILURES =
"logcompaction_failures";
public static final String PENDING_COMPACTIONS_FAILURES =
"pending_compactions_failures";
+ // Metadata table compaction health. The existing per-partition
baseFileCount/logFileCount gauges
Review Comment:
**nit:** Feel free to ignore. `STAT_LAST_COMPACTION_TIMESTAMP` (L79) has
been declared and never emitted since 298808baaf77 (2020-12-30). Since this
block adds a compaction-health gauge next to it, would it be worth either
wiring it up (an age-since-compaction gauge would not freeze on the failure
path noted at `HoodieBackedTableMetadataWriter.java:1556`) or deleting it?
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/metadata/TestHoodieBackedTableMetadataWriter.java:
##########
@@ -651,4 +651,83 @@ void testPerformTableServicesWithFailureHandling(
// Verify metrics are incremented when there's a failure
verify(metrics,
times(1)).incrementMetric(HoodieMetadataMetrics.PENDING_COMPACTIONS_FAILURES,
1);
}
+
+ @Test
+ void performTableServicesReportsCompletedDeltaCommitsSinceLastCompaction()
throws Exception {
+ // A completed compaction, then three completed delta commits after it,
plus one still inflight.
+ // The gauge must count the three completed instants only, so that it
stays comparable to
+ // hoodie.compact.inline.max.delta.commits, which the compaction trigger
evaluates the same way.
+ List<HoodieInstant> instants = new ArrayList<>();
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"001", "0011"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.COMMIT_ACTION, "002",
"0021"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"003", "0031"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"004", "0041"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"005", "0051"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.INFLIGHT, HoodieTimeline.DELTA_COMMIT_ACTION,
"006"));
+
+ HoodieMetadataMetrics metrics = mock(HoodieMetadataMetrics.class);
+ writerForTableServices(metrics,
createMockTimeline(instants)).performTableServices(Option.empty(), true);
+
+ verify(metrics, times(1)).updateDeltaCommitsSinceLastCompaction(3L);
Review Comment:
**minor:** Not blocking. Every delta commit here is requested and completed
on the same side of the compaction's requested time, so this passes identically
under `findInstantsAfter(requestedTime)`; the completion-time semantics
HUDI-2461 (61f35ebe423d) introduced are unpinned. No case reaches 0 through the
main branch either. Could we add `createNewInstant(COMPLETED,
DELTA_COMMIT_ACTION, "0015", "0035")` (requested before "002", completed after)
expecting 4, and a compaction-last case expecting 0?
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/metadata/TestHoodieBackedTableMetadataWriter.java:
##########
@@ -651,4 +651,83 @@ void testPerformTableServicesWithFailureHandling(
// Verify metrics are incremented when there's a failure
verify(metrics,
times(1)).incrementMetric(HoodieMetadataMetrics.PENDING_COMPACTIONS_FAILURES,
1);
}
+
+ @Test
+ void performTableServicesReportsCompletedDeltaCommitsSinceLastCompaction()
throws Exception {
+ // A completed compaction, then three completed delta commits after it,
plus one still inflight.
+ // The gauge must count the three completed instants only, so that it
stays comparable to
+ // hoodie.compact.inline.max.delta.commits, which the compaction trigger
evaluates the same way.
+ List<HoodieInstant> instants = new ArrayList<>();
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"001", "0011"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.COMMIT_ACTION, "002",
"0021"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"003", "0031"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"004", "0041"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"005", "0051"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.INFLIGHT, HoodieTimeline.DELTA_COMMIT_ACTION,
"006"));
+
+ HoodieMetadataMetrics metrics = mock(HoodieMetadataMetrics.class);
+ writerForTableServices(metrics,
createMockTimeline(instants)).performTableServices(Option.empty(), true);
+
+ verify(metrics, times(1)).updateDeltaCommitsSinceLastCompaction(3L);
+ }
+
+ @Test
+ void performTableServicesReportsAllDeltaCommitsWhenNeverCompacted() throws
Exception {
Review Comment:
**minor:** Not blocking. This scenario is already asserted
instant-by-instant in
`TestCompactionUtils.testGetDeltaCommitsSinceLatestCompaction`
(`hudi-hadoop-common/.../TestCompactionUtils.java:258`, `prepareTimeline`
L692-705), and it adds no wiring coverage over the sibling above. Could we fold
the three `performTableServicesReports*` tests into one `@ParameterizedTest`
over `(instants, expectedCount)`, as this file does at L135 and L532, dropping
this row?
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/metadata/TestHoodieBackedTableMetadataWriter.java:
##########
@@ -651,4 +651,83 @@ void testPerformTableServicesWithFailureHandling(
// Verify metrics are incremented when there's a failure
verify(metrics,
times(1)).incrementMetric(HoodieMetadataMetrics.PENDING_COMPACTIONS_FAILURES,
1);
}
+
+ @Test
+ void performTableServicesReportsCompletedDeltaCommitsSinceLastCompaction()
throws Exception {
+ // A completed compaction, then three completed delta commits after it,
plus one still inflight.
+ // The gauge must count the three completed instants only, so that it
stays comparable to
+ // hoodie.compact.inline.max.delta.commits, which the compaction trigger
evaluates the same way.
+ List<HoodieInstant> instants = new ArrayList<>();
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"001", "0011"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.COMMIT_ACTION, "002",
"0021"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"003", "0031"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"004", "0041"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"005", "0051"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.INFLIGHT, HoodieTimeline.DELTA_COMMIT_ACTION,
"006"));
+
+ HoodieMetadataMetrics metrics = mock(HoodieMetadataMetrics.class);
+ writerForTableServices(metrics,
createMockTimeline(instants)).performTableServices(Option.empty(), true);
+
+ verify(metrics, times(1)).updateDeltaCommitsSinceLastCompaction(3L);
+ }
+
+ @Test
+ void performTableServicesReportsAllDeltaCommitsWhenNeverCompacted() throws
Exception {
+ // No compaction has ever run, so every completed delta commit counts as
backlog.
+ List<HoodieInstant> instants = new ArrayList<>();
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"001", "0011"));
+ instants.add(INSTANT_GENERATOR.createNewInstant(
+ HoodieInstant.State.COMPLETED, HoodieTimeline.DELTA_COMMIT_ACTION,
"002", "0021"));
+
+ HoodieMetadataMetrics metrics = mock(HoodieMetadataMetrics.class);
+ writerForTableServices(metrics,
createMockTimeline(instants)).performTableServices(Option.empty(), true);
+
+ verify(metrics, times(1)).updateDeltaCommitsSinceLastCompaction(2L);
+ }
+
+ @Test
+ void performTableServicesReportsZeroDeltaCommitsOnEmptyTimeline() throws
Exception {
+ HoodieMetadataMetrics metrics = mock(HoodieMetadataMetrics.class);
+ writerForTableServices(metrics, createMockTimeline(new ArrayList<>()))
+ .performTableServices(Option.empty(), true);
+
+ // No completed delta commit yet, so the gauge is reset rather than left
at a stale value.
+ verify(metrics, times(1)).updateDeltaCommitsSinceLastCompaction(0L);
+ }
+
+ /**
+ * Builds a partially mocked writer whose {@code performTableServices} runs
for real against the given
+ * metadata timeline.
+ */
+ private static HoodieBackedTableMetadataWriter
writerForTableServices(HoodieMetadataMetrics metrics,
Review Comment:
**minor:** Not blocking. This is the same setup as
`testPerformTableServicesWithFailureHandling` (L573-640): four mocks, the same
six stubs, the same four field injections and `doCallRealMethod` on
`performTableServices`, except the older test still hand-rolls reflection
instead of `setField` (L518). Could that test be switched onto this helper so
there is one setup path?
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/metadata/TestHoodieBackedTableMetadataWriter.java:
##########
@@ -523,7 +523,7 @@ private static void setField(Object target, String name,
Object value) throws Ex
}
@SuppressWarnings("deprecation")
- private HoodieActiveTimeline createMockTimeline(List<HoodieInstant>
instants) {
+ private static HoodieActiveTimeline createMockTimeline(List<HoodieInstant>
instants) {
Review Comment:
**nit:** Feel free to ignore. All four callers of `createMockTimeline` are
instance `@Test` methods and `writerForTableServices` never calls it, so the
`static` here is unneeded churn. Could we revert this line?
```suggestion
private HoodieActiveTimeline createMockTimeline(List<HoodieInstant>
instants) {
```
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/metadata/TestHoodieBackedTableMetadataWriter.java:
##########
@@ -651,4 +651,83 @@ void testPerformTableServicesWithFailureHandling(
// Verify metrics are incremented when there's a failure
verify(metrics,
times(1)).incrementMetric(HoodieMetadataMetrics.PENDING_COMPACTIONS_FAILURES,
1);
}
+
+ @Test
+ void performTableServicesReportsCompletedDeltaCommitsSinceLastCompaction()
throws Exception {
+ // A completed compaction, then three completed delta commits after it,
plus one still inflight.
+ // The gauge must count the three completed instants only, so that it
stays comparable to
+ // hoodie.compact.inline.max.delta.commits, which the compaction trigger
evaluates the same way.
Review Comment:
**nit:** Feel free to ignore. This names
`hoodie.compact.inline.max.delta.commits`, while the javadoc on
`updateDeltaCommitsSinceLastCompaction` names
`hoodie.metadata.compact.max.delta.commits` (`HoodieMetadataConfig.java:141`,
mapped onto the inline knob for the MDT write config at
`HoodieMetadataWriteUtils.java:301`). Could we use the metadata key here too?
```suggestion
// hoodie.metadata.compact.max.delta.commits, which the compaction
trigger evaluates the same way.
```
--
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]
