You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "yihua (via GitHub)" <gi...@apache.org> on 2026/04/04 17:39:09 UTC
Re: [PR] feat: Adding support to block archival on last known ECTR for v6 tables [hudi]
yihua commented on code in PR #18380:
URL: https://github.com/apache/hudi/pull/18380#discussion_r3035843495
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/timeline/versioning/v1/TimelineArchiverV1.java:
##########
@@ -271,7 +271,30 @@ private Stream<HoodieInstant> getCommitInstantsToArchive() throws IOException {
Option<HoodieInstant> oldestInstantToRetainForClustering =
ClusteringUtils.getEarliestInstantToRetainForClustering(table.getActiveTimeline(), table.getMetaClient(), config.getCleanerPolicy());
+ // If enabled, block archival based on ECTR from the last completed clean to ensure we don't archive
+ // commits that have data files that haven't been cleaned yet.
+ Option<HoodieInstant> oldestInstantToRetainForClean = Option.empty();
+ if (config.shouldBlockArchivalOnCleanECTR()) {
+ Option<HoodieInstant> lastCleanInstant = table.getCleanTimeline().filterCompletedInstants().lastInstant();
Review Comment:
🤖 `table.getCleanTimeline()` delegates to `getActiveTimeline().getCleanerTimeline()`, which only returns cleans on the active timeline. If the last completed clean has itself been archived already, `lastCleanInstant` will be empty and the ECTR check is silently skipped. Could you verify this can't happen, or consider also checking the archived timeline for the most recent clean?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/timeline/versioning/v1/TimelineArchiverV1.java:
##########
@@ -271,7 +271,30 @@ private Stream<HoodieInstant> getCommitInstantsToArchive() throws IOException {
Option<HoodieInstant> oldestInstantToRetainForClustering =
ClusteringUtils.getEarliestInstantToRetainForClustering(table.getActiveTimeline(), table.getMetaClient(), config.getCleanerPolicy());
+ // If enabled, block archival based on ECTR from the last completed clean to ensure we don't archive
+ // commits that have data files that haven't been cleaned yet.
+ Option<HoodieInstant> oldestInstantToRetainForClean = Option.empty();
+ if (config.shouldBlockArchivalOnCleanECTR()) {
+ Option<HoodieInstant> lastCleanInstant = table.getCleanTimeline().filterCompletedInstants().lastInstant();
+ if (lastCleanInstant.isPresent()) {
+ try {
+ org.apache.hudi.avro.model.HoodieCleanMetadata cleanMetadata =
+ table.getActiveTimeline().readCleanMetadata(lastCleanInstant.get());
+ if (cleanMetadata.getEarliestCommitToRetain() != null
+ && !cleanMetadata.getEarliestCommitToRetain().trim().isEmpty()) {
+ oldestInstantToRetainForClean = commitTimeline.findInstantsAfterOrEquals(
+ cleanMetadata.getEarliestCommitToRetain()).firstInstant();
+ log.info("Blocking archival based on ECTR {} from last clean {}",
+ cleanMetadata.getEarliestCommitToRetain(), lastCleanInstant.get().requestedTime());
+ }
Review Comment:
🤖 This catch block silently skips the ECTR check on IOException, which means archival proceeds without the safety guard. Since the whole purpose of this feature is to prevent data leaks from archiving commits with uncleaned data files, shouldn't this fail closed (i.e., throw or block archival) rather than fail open? A transient IO error here could cause exactly the data leak the feature is designed to prevent.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org