feat: Adding support to block archival on last known ECTR for v6 tables#18380
feat: Adding support to block archival on last known ECTR for v6 tables#18380nsivabalan wants to merge 3 commits intoapache:masterfrom
Conversation
yihua
left a comment
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Thanks for contributing! The motivation is solid — preventing data leaks when cleaning configs change between clean and archival is an important safety improvement. However, the current implementation has a fail-open design in both its error handling and timeline scope that could undermine the safety guarantee. Please see the inline comments for details.
nsivabalan
force-pushed
the
archivalBlockECTRV6
branch
from
cbd2f32 to
d4dd819
Compare
nsivabalan
force-pushed
the
archivalBlockECTRV6
branch
from
d4dd819 to
7b2aa41
Compare
yihua
left a comment
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Style & Readability Review — A couple of minor cleanups: remove a commented-out throw statement and dead code that creates an unused Map variable.
yihua
left a comment
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Thanks for contributing! The approach is solid — reading ECTR from the last clean to gate archival is a clean solution to the config-drift problem. One concern: the swallowed IOException in the critical path could silently defeat the safety check (details in inline comment).
yihua
left a comment
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Greptile Summary: This PR introduces an opt-in archival guard for Hudi v6 tables (using TimelineArchiverV1) that reads the Earliest Commit To Retain (ECTR) from the last completed clean operation and prevents archiving any commits at or after that boundary. The feature is gated behind a new config hoodie.archive.block.on.latest.clean.ectr (default false) to maintain full backward compatibility. Five integration tests cover the primary scenarios.
Key changes:
TimelineArchiverV1: reads ECTR from last clean'sHoodieCleanMetadataand adds a filter to the archival stream; the feature is skipped when no clean exists or ECTR is empty/null.HoodieArchivalConfig: newBLOCK_ARCHIVAL_ON_LATEST_CLEAN_ECTRconfig property with builder support.HoodieWriteConfig: newshouldBlockArchivalOnCleanECTR()accessor, plus three unrelated changes (ORC codec import/method, removal ofgetClusteringEarliestCommitToCluster) that should be in a separate PR.- Tests: five tests in
TestHoodieTimelineArchiver; the test namedtestArchivalContinuesWhenCleanMetadataIsMissingonly covers the "no clean commit" path, not theIOExceptionpath its name implies.
Main concern: In the IOException catch block (lines 291–294 of TimelineArchiverV1), the log.warn message says \"skipping ECTR check\" — implying graceful degradation — but the code immediately throws HoodieIOException, aborting the archival. The intent (fail-fast vs. graceful skip) needs to be clarified and the log message corrected to match.
Greptile Confidence Score: 3/5
Not safe to merge as-is — the IOException handling is self-contradictory and could silently fail archival in production when clean metadata is temporarily unreadable.
The core logic (ECTR filter, config, builder, tests) is sound, but the IOException catch block has a direct contradiction: the log says skipping while the code throws, making the failure mode ambiguous and untested. The unrelated changes to HoodieWriteConfig add diff noise and risk. Both issues need resolution before merge.
TimelineArchiverV1.java (IOException handling) and HoodieWriteConfig.java (unrelated changes).
Vulnerabilities
No security concerns identified. The new config is disabled by default, the feature only reads existing clean metadata from the timeline (no external input processed as code), and no credentials, tokens, or sensitive data are involved.
CodeRabbit Walkthrough: This PR introduces an ECTR (Earliest Commit To Retain) based archival blocking mechanism. When enabled, the timeline archiver reads the latest CLEAN metadata to extract the ECTR value and conditionally blocks archival of commit instants predating this threshold, preventing premature removal of essential commits.
Sequence Diagram (CodeRabbit):
sequenceDiagram
participant TimelineArchiver
participant Config
participant Timeline
participant CleanMetadata
participant CommitFilter
TimelineArchiver->>Config: Check shouldBlockArchivalOnCleanECTR()
alt ECTR Blocking Enabled
TimelineArchiver->>Timeline: Read latest CLEAN instant
TimelineArchiver->>CleanMetadata: Parse HoodieCleanMetadata
CleanMetadata-->>TimelineArchiver: getEarliestCommitToRetain()
alt ECTR Value Present
TimelineArchiver->>CommitFilter: Add ECTR-based filter condition
CommitFilter->>CommitFilter: Exclude commits with requestedTime < ECTR
CommitFilter-->>TimelineArchiver: Filtered commit instants
else ECTR Missing/Blank
TimelineArchiver->>CommitFilter: Skip ECTR filter
CommitFilter-->>TimelineArchiver: Unfiltered commit instants
end
else ECTR Blocking Disabled
TimelineArchiver->>CommitFilter: Apply standard archival filters
CommitFilter-->>TimelineArchiver: Filtered commit instants
end
TimelineArchiver->>Timeline: Archive filtered instants
CodeRabbit: yihua#28 (review)
Greptile: yihua#28 (review)
| } catch (IOException e) { | ||
| log.warn("Failed to read clean metadata for {}, skipping ECTR check", lastCleanInstant.get(), e); | ||
| throw new HoodieIOException("Failed to read clean metadata for " + lastCleanInstant.get() + ", skipping ECTR check", e); | ||
| } |
There was a problem hiding this comment.
Failure message contradicts runtime behavior.
Line 292 and Line 293 say “skipping ECTR check,” but the code throws immediately, which aborts archival. Please align the message with actual behavior to avoid operator confusion.
Suggested text fix
- log.warn("Failed to read clean metadata for {}, skipping ECTR check", lastCleanInstant.get(), e);
- throw new HoodieIOException("Failed to read clean metadata for " + lastCleanInstant.get() + ", skipping ECTR check", e);
+ log.warn("Failed to read clean metadata for {}, aborting archival due to ECTR guard", lastCleanInstant.get(), e);
+ throw new HoodieIOException("Failed to read clean metadata for " + lastCleanInstant.get()
+ + ", aborting archival due to ECTR guard", e);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| } catch (IOException e) { | |
| log.warn("Failed to read clean metadata for {}, aborting archival due to ECTR guard", lastCleanInstant.get(), e); | |
| throw new HoodieIOException("Failed to read clean metadata for " + lastCleanInstant.get() | |
| ", aborting archival due to ECTR guard", e); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/timeline/versioning/v1/TimelineArchiverV1.java`
around lines 291 - 294, The catch block in TimelineArchiverV1 currently logs and
throws with a message that says "skipping ECTR check" which is inaccurate
because the code immediately throws and aborts archival; update both the
log.warn call and the HoodieIOException message in the catch(IOException e)
handler so the text reflects that the archival is being aborted (e.g., "aborting
archival due to failure reading clean metadata for <instant>") instead of saying
it will skip the ECTR check, leaving the exception chaining intact.
— CodeRabbit (original) (source:comment#3055869924)
yihua
left a comment
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Style & Readability Review — a few readability and code quality suggestions: consolidate the null/empty check, clarify a comment, avoid duplicating filter logic, and extract repeated test setup.
yihua
left a comment
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Thanks for contributing! The approach to block archival based on ECTR from the last clean is sound and addresses a real data-leak scenario. A couple of items to double-check in the inline comments.
|
@yihua : can you help land this once you are ok. if there are minor cosmetic suggestions, free free to address them and land them. |
yihua
left a comment
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Thanks for addressing the feedback! The contradictory "skipping ECTR check" wording has been removed from both the log message and the exception, which resolves the prior finding cleanly. The fail-closed behavior (throwing on read failure) is now correctly reflected in the messaging.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18380 +/- ##
============================================
+ Coverage 68.36% 68.85% +0.48%
- Complexity 27566 28223 +657
============================================
Files 2432 2460 +28
Lines 133175 135290 +2115
Branches 16023 16399 +376
============================================
+ Hits 91047 93148 +2101
+ Misses 35068 34764 -304
- Partials 7060 7378 +318
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
yihua
left a comment
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Nice updates — the test assertions are now much more precise and exhaustive, replacing vague "some commits were archived" checks with exact verification of which commits remain and which were archived. All prior findings from my previous review were already addressed (the contradictory log/exception messages were fixed in the earlier round), and no new issues are introduced by these test-only changes.
4 participants
Describe the issue this Pull Request addresses
This PR adds support to block archival based on the Earliest Commit To Retain (ECTR) from the last completed clean operation, preventing potential data leaks when cleaning configurations change between clean and archival runs.
Problem: Currently, archival recomputes ECTR independently based on cleaning configs at archival time, rather than reading it from the last clean plan. When cleaning configs change between clean and archival operations, archival may archive commits whose data files haven't been cleaned yet, leading to timeline metadata loss for existing data files.
Example scenario:
Summary and Changelog
User-facing summary: Users can now optionally enable archival blocking based on ECTR from the last clean to prevent archiving commits whose data files haven't been cleaned. This is useful when cleaning configurations may change over time or when strict data retention guarantees are needed.
Detailed changelog:
Configuration Changes:
Implementation Changes:
Test Changes:
a. testArchivalBlocksOnCleanECTRWhenEnabled - Core blocking functionality
b. testArchivalProceedsNormallyWhenECTRBlockingDisabled - Backward compatibility
c. testArchivalMakesProgressWhenECTRIsLaterThanArchivalWindow - Progress validation
d. testArchivalContinuesWhenCleanMetadataIsMissing - Missing metadata handling
e. testArchivalHandlesEmptyECTRInCleanMetadata - Empty ECTR handling
f. testArchivalProceedsWhenCleanHasFileVersionsPolicyWithNullECTR - FILE_VERSIONS policy compatibility
g. testArchivalBlocksOnCleanECTRWithTimelineArchiverV2AndVersion9 - Version 9 / LSM timeline compatibility
Impact
Public API Changes:
User-facing changes:
Performance impact:
Breaking changes: None - opt-in feature with no default behavior changes
Risk Level
low
Documentation Update
Config documentation:
The new config hoodie.archive.block.on.latest.clean.ectr is documented inline:
.withDocumentation("If enabled, archival will block on latest ECTR from last known clean")
Website documentation needed:
Contributor's checklist