shangxinli commented on code in PR #19868:
URL: https://github.com/apache/hudi/pull/19868#discussion_r3991768247
##########
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:
Confirmed, and moved it into the `finally` block. One correction to the
suggestion though: `getActiveTimeline()` there is not reliable either, since
every caller passes `requiresTimelineRefresh=false`
(`HoodieTableMetadataWriter.java:178`) and neither `runAnyPendingCompactions`
nor `writeClient.compact` refreshes this meta client. Sampling off
`reloadActiveTimeline()` instead, wrapped so a metric failure cannot mask the
original exception.
##########
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:
Right as it stood, but moving the sample into `finally` drops the
`lastInstant` guard from that path, so an empty delta-commit timeline now
reaches it. Keeping `.orElse(0L)` as the real zero case.
##########
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:
Correct, reworded: it plateaus near `hoodie.keep.min.commits` once archival
stops pinning the last compaction. I left the 2x out though, since the plateau
depends on the archival config as much as on the compaction threshold. With the
sample now post-service a healthy table returns to about zero each cycle, so
the guidance is "stays at or above the threshold across consecutive cycles".
##########
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:
Good catch, verified at `CloudWatchReporter.java:282`. Now prefixed with the
MDT table name at the call site, under the same `StringUtils.nonEmpty` guard as
`TABLE_SERVICE_EXECUTION_*`. Worth noting `Metrics#registerGauge` applies no
prefix of its own, only `registerGauges` does.
##########
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:
Dropped the three size gauges from this PR, so this no longer applies here.
The gap is real and pre-existing though: `updateSizeMetrics` should be covered
in `testMetadataMetrics` regardless, and I am happy to file that separately.
##########
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:
Your worked case is right, a sum ratio is dominated by whichever partition
has the most file groups. Rather than switch to a max I dropped the ratio,
since the per-partition counts are more actionable than either aggregate.
--
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]
