hudi-agent commented on code in PR #19868:
URL: https://github.com/apache/hudi/pull/19868#discussion_r3991861902
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -1570,13 +1570,47 @@ public void performTableServices(Option<String>
inFlightInstantTimestamp, boolea
String executionStatusMetricName = tableNameExists
? String.format("%s.%s", metadataTableName,
HoodieMetadataMetrics.TABLE_SERVICE_EXECUTION_STATUS)
: HoodieMetadataMetrics.TABLE_SERVICE_EXECUTION_STATUS;
+ String deltaCommitsMetricName = tableNameExists
+ ? String.format("%s.%s", metadataTableName,
HoodieMetadataMetrics.STAT_DELTA_COMMITS_SINCE_LAST_COMPACTION)
+ : HoodieMetadataMetrics.STAT_DELTA_COMMITS_SINCE_LAST_COMPACTION;
long timeSpent = metadataTableServicesTimer.endTimer();
metrics.ifPresent(m -> m.setMetric(executionDurationMetricName,
timeSpent));
if (allTableServicesExecutedSuccessfullyOrSkipped) {
metrics.ifPresent(m -> m.setMetric(executionStatusMetricName, 1));
} else {
metrics.ifPresent(m -> m.setMetric(executionStatusMetricName, -1));
}
+ reportDeltaCommitsSinceLastCompaction(deltaCommitsMetricName);
+ }
+ }
+
+ /**
+ * Reports the metadata table's compaction backlog, so that a stalled
compaction is visible before the
+ * growing log to base file ratio shows up as slower metadata lookups.
+ *
+ * <p>Sampled here rather than alongside the compaction above so that the
failure paths report too: a
+ * pending compaction that keeps failing throws out of
+ * {@link #runPendingTableServicesOperationsAndRefreshTimeline} before
compaction is even reached, which
+ * is exactly the state this gauge exists to surface.
+ *
+ * <p>The timeline is reloaded rather than reused. Callers pass {@code
requiresTimelineRefresh=false}
+ * ({@code HoodieTableMetadataWriter#performTableServices}), and neither
{@code runAnyPendingCompactions}
+ * nor {@code writeClient.compact} refreshes this meta client - both go
through their own - so the cached
+ * timeline can predate both the failure and the compaction being reported
on.
+ */
+ private void reportDeltaCommitsSinceLastCompaction(String metricName) {
+ if (!metrics.isPresent()) {
+ return;
+ }
+ try {
Review Comment:
🤖 nit: the rest of this method uses `metrics.ifPresent(m -> ...)`; could you
use the same style here instead of the explicit `isPresent()`/`get()` pair for
consistency?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataMetrics.java:
##########
@@ -84,6 +84,26 @@ 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. This is
+ // the count of completed delta commits since the last completed compaction,
counting the same instants
+ // that ScheduleCompactionActionExecutor compares against
hoodie.metadata.compact.max.delta.commits.
+ //
+ // It is sampled after table services have run, so a healthy table reports
close to zero once a
+ // compaction lands, and a table whose compaction is failing or is never
scheduled keeps reporting a
+ // backlog at or above the threshold. Alert on it staying at or above the
threshold across consecutive
+ // cycles rather than on a fixed multiple of it: the value does not grow
without bound. Once the backlog
+ // reaches the threshold the archiver stops pinning the last compaction
+ // (CompactionUtils#getEarliestInstantToRetainForCompaction), MDT archival
eventually removes it, and the
+ // count then plateaus near hoodie.keep.min.commits, which depends on the
archival config as well.
+ //
+ // It is emitted from HoodieBackedTableMetadataWriter#performTableServices,
so it covers table services
+ // driven through that path. Under hoodie.metadata.streaming.write.enabled,
Flink runs MDT compaction in
+ // a separate pipeline that does not call it, and this gauge is not emitted
there. Read it alongside
+ // TABLE_SERVICE_EXECUTION_STATUS and COMPACTION_FAILURES, which distinguish
a failing compaction from
+ // table services having stopped running at all.
+ public static final String STAT_DELTA_COMMITS_SINCE_LAST_COMPACTION =
"deltaCommitsSinceLastCompaction";
+
private final transient MetricRegistry metricsRegistry;
private final transient Metrics metrics;
Review Comment:
🤖 nit: the neighboring constants in this block
(TABLE_SERVICE_EXECUTION_STATUS, PENDING_COMPACTIONS_FAILURES, etc.) don't use
a `STAT_` prefix — could you drop it here too for consistency?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
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:
🤖 Worth noting this nit flipped with the revision: now that the sample runs
from `finally` with no early return, `getDeltaCommitsSinceLatestCompaction`
does return empty for an MDT with no delta commits yet
(`CompactionUtils.java:329-335`), so `.orElse(0L)` is reachable — the new
"empty timeline" parameterized case asserts exactly that path. Switching to
`.get()` here would throw on a fresh MDT, so the fallback should stay.
--
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]
