feat(rollback): add RollbackOrphanDetector utility for archive orphan guard (#18783)#18795
feat(rollback): add RollbackOrphanDetector utility for archive orphan guard (#18783)#18795shangxinli wants to merge 1 commit into
Conversation
…chive guard Introduces RollbackOrphanDetector and a feature-flag config that will later gate archival of rollback instants when their orphan files are still on storage. No behavior change yet — this PR only lands the building block and config; wiring into the archive planner follows in a separate PR. See apache#18783 for the motivating problem: when a rollback partially fails (crash mid-rollback, marker loss, or a blocked storage close() that lands data after rollback completed) and the rollback instant is later archived, the system loses the metadata anchor that lets readers filter out the orphan files, leading to corrupt-parquet errors or duplicate records. Two detection modes: - LIGHT : reads HoodieRollbackMetadata.failedDeleteFiles. - THOROUGH : additionally lists partitions named in the rollback metadata and matches filenames against the target instant time(s). Catches late-landing writes. A safety floor cross-checks every candidate against the completed timeline so a legitimate committed file with a matching filename is never flagged. Config: hoodie.archive.rollback.orphan.guard.mode = OFF | LIGHT | THOROUGH Default: OFF (no behavior change). Test: TestRollbackOrphanDetector covers OFF / LIGHT / THOROUGH and the safety-floor case.
shangxinli
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18795 +/- ##
============================================
+ Coverage 68.22% 68.25% +0.02%
- Complexity 29290 29357 +67
============================================
Files 2525 2528 +3
Lines 141733 141969 +236
Branches 17614 17650 +36
============================================
+ Hits 96698 96899 +201
- Misses 37065 37086 +21
- Partials 7970 7984 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hudi-agent
left a comment
hudi-agent
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 working on this! The PR introduces a RollbackOrphanDetector utility that's off-by-default and scoped as foundation for a follow-up archive-precondition PR — that staging keeps blast radius low. The main concern from this pass is the log-file regex in THOROUGH mode (and the matching INSTANT_REGEX_LOG constant): it appears to inspect the writeToken position rather than the embedded instant, so MoR log-file detection likely doesn't behave as the Javadoc suggests. A couple of smaller items (Javadoc/impl mismatch on hasOrphans short-circuit, and a locale concern in Mode.parse) are noted inline. Please take a look at the inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. A few naming and documentation accuracy nits in RollbackOrphanDetector.java.
| private static final Logger LOG = LoggerFactory.getLogger(RollbackOrphanDetector.class); | ||
|
|
||
| /** Matches Hudi base/log filenames carrying an embedded instant time at the trailing position. */ | ||
| private static final String INSTANT_REGEX_BASE = "_(\\d+)\\.parquet$"; |
There was a problem hiding this comment.
🤖 I think this log regex (and the matching one in buildPatterns below) inspects the wrong position. Hudi log files are named .<fileId>_<deltaCommitTime>.log.<version>_<writeToken> per FSUtils.LOG_FILE_PATTERN, with writeToken formatted as <digits>-<digits>-<digits> (e.g. 0-1-0). The current pattern \.log\.\d+_(\d+)(?:_[^/]*)?$ matches digits AFTER .log.<version>_, i.e. the writeToken position — for a typical name like .fileId_20260101000000000.log.1_0-1-0 it wouldn't match at all (trailing -1-0 blocks $), and for the rare _0 writeToken it would capture 0, not the instant. Either way, late-landing log files won't be detected in THOROUGH mode (the test only exercises parquet via createBaseFileToRollback, so this isn't caught). Did you mean something like _(\d+)\.log\.\d+(?:_[^/]*)?$ here, with the same fix to the buildPatterns variant? @yihua could you sanity-check the intended log-file matching here?
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| } | ||
|
|
||
| /** | ||
| * Returns true iff any orphan files are detected for {@code rollbackInstant}. |
There was a problem hiding this comment.
🤖 The Javadoc says hasOrphans is 'Optimised to short-circuit as soon as the first orphan is found in THOROUGH', but the implementation below just calls detectOrphans(...).isEmpty() — which still lists every partition in the rollback metadata. Either the Javadoc should be relaxed, or hasOrphans should actually break out of the partition loop on first hit. For an archive-precondition call site, the difference can matter on rollbacks touching many partitions.
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| if (value == null || value.isEmpty()) { | ||
| return OFF; | ||
| } | ||
| try { |
There was a problem hiding this comment.
🤖 Minor: toUpperCase() without a Locale uses the default locale. In a Turkish-locale JVM, 'light'.toUpperCase() produces LİGHT (dotted I), and Mode.valueOf would then fall back to OFF. Could you use toUpperCase(Locale.ROOT) here for safety?
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| * <p>A safety floor cross-checks every candidate against the completed | ||
| * timeline: a file whose embedded instant appears as a {@code COMPLETED} | ||
| * commit in either the active or archived timeline is never flagged as an | ||
| * orphan, even if its filename matches the regex. |
There was a problem hiding this comment.
🤖 nit: the class-level Javadoc says the safety floor checks "either the active or archived timeline", but the implementation in filterAgainstCompletedTimeline only examines the active timeline (and the inline comment there even explains why). Could you update this to say "the active timeline" so the documented guarantee matches what's actually implemented?
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| for (HoodieRollbackPartitionMetadata pm : metadata.getPartitionMetadata().values()) { | ||
| List<String> ff = pm.getFailedDeleteFiles(); | ||
| if (ff != null) { | ||
| failed.addAll(ff); |
There was a problem hiding this comment.
🤖 nit: ff is pretty terse here — could you rename it to failedFiles or failedDeleteFiles to make it easier to follow at a glance?
- AI-generated; verify before applying. React 👍/👎 to flag quality.
| String name = path.substring(path.lastIndexOf('/') + 1); | ||
| java.util.regex.Matcher m = Pattern.compile(INSTANT_REGEX_BASE).matcher(name); | ||
| if (m.find()) { | ||
| return m.group(1); |
There was a problem hiding this comment.
🤖 nit: java.util.regex.Matcher is used with a fully-qualified name here even though Pattern is already imported — worth adding import java.util.regex.Matcher;. Also, since INSTANT_REGEX_BASE and INSTANT_REGEX_LOG are constants, promoting them to pre-compiled static final Pattern fields would avoid recompiling the same patterns on every path checked.
- AI-generated; verify before applying. React 👍/👎 to flag quality.
|
responded in the github issue. @danny0405 and @cshuo tried simulating this and ran flink jobs for hours and we could not reproduce the issue. So, using oss hudi, oss gcs, the issue is not a valid one. Likely you have some custom hudi or custom GCS internally deployed in your org which could cause the lingering files issue. If you suspect its a valid oss bug, can you help us w/ a reproducible bug and we can take it from there. |
5 participants
Describe the issue this Pull Request addresses
See issue #18783 for the full problem statement. In summary: when a rollback partially fails (crash mid-rollback, marker loss, or a blocked storage
close()that lands data after rollback completed) and the rollback instant is later archived, the system loses the metadata anchor that lets readers filter out the orphan files. Readers then return corrupt-parquet errors or duplicate records — a hard violation of the reader/writer isolation guarantee.This PR is the foundation for an archive-time precondition check that prevents that loss-of-anchor scenario. It introduces a
RollbackOrphanDetectorand a feature-flag config; the actual wiring into the archive planner is a follow-up cascade PR. No behavior change yet.Related:
feat/rollback-orphan-archive-preconditionon the fork; will be opened after this mergesfeat/rollback-orphan-repair-clion the fork; will be opened after this mergesSummary and Changelog
A new
RollbackOrphanDetectorutility plus the config that will eventually gate archival of rollback instants when their orphan files are still on storage. No behavior change yet — the config defaults toOFF.RollbackOrphanDetectorinhudi-client/hudi-client-commonunderorg.apache.hudi.table.action.rollback. Two detection modes:LIGHT— readsHoodieRollbackMetadata.failedDeleteFiles. O(metadata size). Catches files the rollback explicitly tried and failed to delete but misses post-rollback late landings.THOROUGH— additionally lists the partitions named in the rollback metadata and matches filenames against the rollback's target instant time(s) (both base parquet and MoR log file naming). Bounded by partition count in the rollback metadata, not whole-table size.COMPLETEDcommit is never flagged as an orphan, even if its filename matches the regex.hoodie.archive.rollback.orphan.guard.mode(valuesOFF/LIGHT/THOROUGH, defaultOFF) and a getter onHoodieWriteConfig.(HoodieTable, HoodieInstant, Mode)for the archive planner context and(HoodieTableMetaClient, HoodieInstant, Mode)for thehudi-clicontext that follows in PR3.TestRollbackOrphanDetectorwith 5 tests coveringOFF,LIGHTwith empty/non-emptyfailedDeleteFiles,THOROUGHwith a real partition listing, and the safety-floor case.Impact
None until the new config is set to
LIGHTorTHOROUGH. The defaultOFFshort-circuits before any work happens.Risk Level
low
Documentation Update
A user-facing config doc update is appropriate once the cascade PR (which actually wires the detector in) lands — that's the change users would actually flip. This PR is plumbing only.
Contributor's checklist