nsivabalan commented on code in PR #20151: URL: https://github.com/apache/hudi/pull/20151#discussion_r4279812704
########## hudi-utilities/src/main/java/org/apache/hudi/utilities/health/CleanerCadenceCheck.java: ########## @@ -0,0 +1,151 @@ +/* + * 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.avro.model.HoodieCleanMetadata; +import org.apache.hudi.common.table.timeline.HoodieActiveTimeline; +import org.apache.hudi.common.table.timeline.HoodieInstant; +import org.apache.hudi.common.table.timeline.HoodieTimeline; +import org.apache.hudi.common.util.CleanerUtils; +import org.apache.hudi.common.util.Option; +import org.apache.hudi.config.HoodieCleanConfig; + +/** + * Checks whether the cleaner is keeping pace with ingestion. + * + * <p>Cleaning is what releases storage: every commit leaves behind older file versions that stay on + * disk until a clean removes them. When the cleaner stops, the table keeps working and nothing + * errors -- storage simply grows, and with it the cost of every listing. Like the other cadence + * checks this one asks not "has a clean run recently" but whether write commits have accumulated + * well past the point where the configured trigger should have scheduled one. + * + * <p>Two allowances are made before calling the table unhealthy. The configured trigger is + * multiplied by a slack factor, since async cleaning trails ingestion by design. And a floor is + * applied on top, because the trigger defaults to one commit: without a floor, a cleaner that is + * two commits behind -- routine when cleaning is asynchronous or briefly loses a lock to a + * concurrent writer -- would be reported as a failure. + */ +public class CleanerCadenceCheck implements HealthCheck { + + static final String NAME = "cleaner"; + + /** Multiplier on the configured trigger, absorbing async cleaning running on its own cadence. */ + static final double TRIGGER_SLACK_FACTOR = 2.0; + + /** + * Minimum number of commits since the last clean before the table can be called unhealthy, + * regardless of how low the trigger is set. Keeps a trigger of one commit from turning ordinary + * async lag into an alert. + */ + static final int MIN_UNHEALTHY_COMMITS = 5; + + @Override + public String getName() { + return NAME; + } + + @Override + public String getDescription() { + return "Whether the cleaner is keeping pace with write commits"; + } + + @Override + public HealthCheckResult check(HealthCheckContext context) throws Exception { + Option<String> missing = context.firstMissingConfig(HoodieCleanConfig.CLEAN_TRIGGER_MAX_COMMITS.key()); + if (missing.isPresent()) { + return context.skipForMissingConfig(NAME, missing.get()); + } + + int triggerCommits = Integer.parseInt(context.getProps().getString( + HoodieCleanConfig.CLEAN_TRIGGER_MAX_COMMITS.key(), + HoodieCleanConfig.CLEAN_TRIGGER_MAX_COMMITS.defaultValue())); + int threshold = Math.max((int) Math.ceil(TRIGGER_SLACK_FACTOR * triggerCommits), MIN_UNHEALTHY_COMMITS); + + HoodieActiveTimeline activeTimeline = context.getMetaClient().getActiveTimeline(); + HoodieTimeline completedWrites = activeTimeline.getCommitsTimeline().filterCompletedInstants(); + Option<HoodieInstant> lastClean = activeTimeline.getCleanerTimeline().filterCompletedInstants().lastInstant(); + + HealthCheckResult.Builder result = HealthCheckResult.builder(NAME) + .config(HoodieCleanConfig.CLEAN_TRIGGER_MAX_COMMITS.key(), triggerCommits) + .config("effective.unhealthy.threshold.commits", threshold) + .config("observed.completed.write.commits", completedWrites.countInstants()); + echoIfSupplied(context, result, HoodieCleanConfig.CLEANER_POLICY.key()); + echoIfSupplied(context, result, HoodieCleanConfig.CLEANER_COMMITS_RETAINED.key()); + + int commitsSinceClean; + if (lastClean.isPresent()) { + commitsSinceClean = completedWrites.findInstantsAfter(lastClean.get().requestedTime()).countInstants(); + result.config("observed.last.clean.instant", lastClean.get().requestedTime()); + describeLastClean(context, lastClean.get(), result); + } else { + commitsSinceClean = completedWrites.countInstants(); + result.config("observed.last.clean.instant", "none"); + } + result.config("observed.commits.since.last.clean", commitsSinceClean); + + if (commitsSinceClean <= threshold) { + return result.status(HealthStatus.HEALTHY) + .summary(lastClean.isPresent() + ? String.format("%d commit(s) since clean at %s, within the threshold of %d", + commitsSinceClean, lastClean.get().requestedTime(), threshold) + : String.format("%d commit(s) and no clean yet, within the threshold of %d", + commitsSinceClean, threshold)) + .build(); + } + + result.status(HealthStatus.UNHEALTHY) + .summary(lastClean.isPresent() + ? String.format("%d commit(s) have accumulated since clean at %s, past the threshold of %d", + commitsSinceClean, lastClean.get().requestedTime(), threshold) + : String.format("%d commit(s) have accumulated and the table has never been cleaned, past the threshold of %d", + commitsSinceClean, threshold)) + .finding(String.format( + "The cleaner is configured to run every %d commit(s); %d have landed without one completing.", + triggerCommits, commitsSinceClean)); + + if (activeTimeline.getCleanerTimeline().filterInflightsAndRequested().countInstants() > 0) { + result.finding("A clean is scheduled or in flight but has not completed. Look for a failing or " + + "stuck clean in the writer logs; a clean that repeatedly fails to acquire the table lock " + + "under concurrent writers is a common cause."); + } else { + result.finding("No clean is scheduled. Check that cleaning is enabled for this table (" Review Comment: Fixed, and this was the sharpest of the findings — the check was blaming the service for what is a configuration conflict. Under `KEEP_LATEST_COMMITS` the cleaner only acts once it can see more than `hoodie.clean.commits.retained` completed commits. If `hoodie.keep.min.commits` is at or below that, archival trims the timeline first and the cleaner never reaches its trigger: file versions pile up and no clean is ever scheduled, exactly as you saw with 60 versions and zero cleans. The check now detects that pairing and names it before falling back to the generic finding: > `hoodie.keep.min.commits=20` is not greater than `hoodie.clean.commits.retained=30`, so archival trims the active timeline before the cleaner has enough commits to act on. Raise `hoodie.keep.min.commits` above 30. Until then file versions accumulate and no clean is scheduled, whatever `hoodie.clean.automatic` is set to. Two tests cover it: the starved pairing produces that finding and *not* the `hoodie.clean.automatic` one, and a healthy pairing still gets the generic finding so the diagnosis doesn't fire spuriously. ########## 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); Review Comment: Yes — same root cause as the thread above, and the `KEEP_LATEST_BY_HOURS` reproduction is the clearer demonstration of it, since there the cleaner's retention has no commit-count expression at all. The threshold now reuses the calculation rather than the raw config. `CleanerUtils.getEarliestCommitToRetain` is called with the table's actual cleaner policy, so `KEEP_LATEST_BY_HOURS` places its window by time exactly as the cleaner does, and the effective bound follows from where that lands. A direct call to `getMinAndMaxInstantsToKeep` isn't possible here — it takes a `HoodieTable` — so the arithmetic is reproduced with a pointer to the original. On the finding pointing at savepoints: that was the closing finding firing regardless of cause. It now names the earliest pending write instant when there is one, and otherwise lists compaction retention and MDT compaction alongside savepoints rather than leading with savepoints. Worth flagging a bug this surfaced: the earliest-commit-to-retain lookup parsed the latest commit's instant time unconditionally, but only `KEEP_LATEST_BY_HOURS` needs it as wall-clock. An instant time that wouldn't parse silently disabled the whole derivation and fell back to the configured bound — which is exactly the failure you'd reported, just reached a different way. Now parsed only for the policy that needs it. -- 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]
