nsivabalan commented on code in PR #20151: URL: https://github.com/apache/hudi/pull/20151#discussion_r4279809301
########## hudi-utilities/src/main/java/org/apache/hudi/utilities/health/ArchivalCadenceCheck.java: ########## @@ -0,0 +1,127 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hudi.utilities.health; + +import org.apache.hudi.common.table.timeline.HoodieActiveTimeline; +import org.apache.hudi.common.util.Option; +import org.apache.hudi.config.HoodieArchivalConfig; + +/** + * Checks whether archival is keeping the active timeline bounded. + * + * <p>Archival moves old instants out of the active timeline into the archived timeline. When it + * stops, the active timeline grows without limit, and because it is read and parsed on essentially + * every table operation, everything touching the table gets progressively slower. Unlike a failed + * write this produces no error -- the table simply degrades. + * + * <p>Two independent signals are reported. The first compares the active timeline against the + * configured retention, with slack: archival runs periodically rather than on every commit, so a + * timeline slightly over the configured maximum is normal. The second is an absolute ceiling, + * because past a few thousand instants timeline loading is a problem regardless of what the + * configuration permits -- a table configured to retain a very large number of commits is + * misconfigured rather than healthy. + * + * <p>This check deliberately does not attribute the cause. A blocked archival is most often a + * savepoint, which {@link SavepointArchivalBlockCheck} diagnoses specifically; the two are meant to + * be read together. + */ +public class ArchivalCadenceCheck implements HealthCheck { + + static final String NAME = "archival"; + + /** + * How far past the configured retention the active timeline may grow before being called + * unhealthy. Archival is periodic, so the timeline routinely sits somewhat above the maximum + * between runs. + */ + static final double RETENTION_SLACK_FACTOR = 1.1; + + /** + * Active timeline size past which the table is called unhealthy regardless of configuration. + * Beyond roughly this many instants, loading and parsing the timeline is itself a performance + * problem on every operation against the table. + */ + static final int MAX_REASONABLE_ACTIVE_INSTANTS = 5000; + + @Override + public String getName() { + return NAME; + } + + @Override + public String getDescription() { + return "Whether archival is keeping the active timeline bounded"; + } + + @Override + public HealthCheckResult check(HealthCheckContext context) { + Option<String> missing = context.firstMissingConfig(HoodieArchivalConfig.MAX_COMMITS_TO_KEEP.key()); + if (missing.isPresent()) { + return context.skipForMissingConfig(NAME, missing.get()); + } + + int maxCommitsToKeep = Integer.parseInt(context.getProps().getString( + HoodieArchivalConfig.MAX_COMMITS_TO_KEEP.key(), + HoodieArchivalConfig.MAX_COMMITS_TO_KEEP.defaultValue())); + + HoodieActiveTimeline activeTimeline = context.getMetaClient().getActiveTimeline(); + int completedInstants = activeTimeline.filterCompletedInstants().countInstants(); + int writeInstants = activeTimeline.getWriteTimeline().countInstants(); + int retentionThreshold = (int) Math.ceil(RETENTION_SLACK_FACTOR * maxCommitsToKeep); + + HealthCheckResult.Builder result = HealthCheckResult.builder(NAME) + .config(HoodieArchivalConfig.MAX_COMMITS_TO_KEEP.key(), maxCommitsToKeep) + .config("effective.unhealthy.threshold.write.instants", retentionThreshold) + .config("effective.absolute.ceiling.completed.instants", MAX_REASONABLE_ACTIVE_INSTANTS) + .config("observed.write.instants", writeInstants) + .config("observed.completed.instants", completedInstants); + + boolean overAbsoluteCeiling = completedInstants >= MAX_REASONABLE_ACTIVE_INSTANTS; + boolean overConfiguredRetention = writeInstants >= retentionThreshold; Review Comment: You're right, and both reproductions were real. The check compared against the raw `hoodie.keep.max.commits` and never looked at the cleaner at all. It now reproduces `ArchivalUtils.getMinAndMaxInstantsToKeep`: derive the cleaner's earliest-commit-to-retain via `CleanerUtils.getEarliestCommitToRetain`, compute `minInstantsToKeepBasedOnCleaning`, and raise the bound to `minBasedOnCleaning + max - min` when the cleaner is holding more history than archival would. It can't call `ArchivalUtils` directly — that takes a `HoodieTable` and this tool reads only the timeline — so the calculation is reproduced, with a comment saying so and a fallback to the configured bound when the cleaner's retention can't be determined. `effective.max.commits.to.keep` is echoed in the output so a verdict can be checked against it. Your `clean.commits.retained=100` / `keep.max.commits=30` case is now a regression test and comes out HEALTHY. Also fixed the counting: you were right that `getWriteTimeline()` includes requested and inflight compaction and clustering, which archival hasn't had the chance to consider. It now counts completed commits, matching what the archiver reasons over. On the other blockers — the earliest pending write instant is visible from the timeline, so when one is present the finding names it with its action and state, since that's the thing an operator can act on. MOR compaction retention and the MDT's latest compaction aren't visible here, so the closing finding now says archival also holds back for those rather than implying a savepoint is the only cause. -- 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]
