zhaoyudi-creator commented on issue #19774: URL: https://github.com/apache/hudi/issues/19774#issuecomment-5618546267
@danny0405 > [@danny0405](https://github.com/danny0405) I'd like to break uncommitted file slices into two categories and check whether the distinction holds up: > > Category 1 — the base instant corresponds to a base file. This is exactly the case you raised: "an uncommitted base file at t2 (leftover from a failed compaction / bulk_insert, not rolled back under LAZY) sharing a file group with later committed delta logs." I agree this one must stay as-is and remain hidden: the pre-V8 read path doesn't run filterUncommittedFiles, and filterBaseFileAfterPendingCompaction only covers pending compaction, so nothing can strip an orphan base file — once the slice is admitted, the base file is read in full. (And under the BUCKET index the fileId is fixed by the bucket number, so later committed writes to the same bucket necessarily land in the same file group as the orphan base file — which makes this leak reachable in exactly the configuration this issue reports.) > > Category 2 — log files only, no base file. This is the case reported here. Even though the base instant (the earliest, failed log's instant) is uncommitted, admitting this slice doesn't leak anything: there's no base file, and any uncommitted log block is filtered out block-by-block by the log reader against the completed timeline (BaseHoodieLogRecordReader skips blocks that are inflight / not on the completed timeline). > > So I'm wondering whether we could use getBaseFile().isPresent() to separate the two and only admit the second: > > private boolean isFileSliceCommitted(FileSlice slice) { > if (!compareTimestamps(slice.getBaseInstantTime(), LESSER_THAN_OR_EQUALS, lastInstant.get().requestedTime())) { > return false; > } > if (timeline.containsOrBeforeTimelineStarts(slice.getBaseInstantTime())) { > return true; > } > if (slice.getBaseFile().isPresent()) { > return false; // Category 1: an uncommitted slice that carries a base file keeps its original semantics — never revived by a log > } > return slice.getLogFiles() > .anyMatch(lf -> timeline.containsOrBeforeTimelineStarts(lf.getDeltaCommitTime())); > } > For any slice that has a base file this is byte-for-byte identical to the pre-patch behavior, so the pre-V8 base-file exposure you're worried about is kept out by this guard; only log-only slices go through the committed-log path. > > A few things I'd appreciate your take on: > > Is this "don't admit a slice that carries a base file" guard enough to cover the inconsistent pre-V8 slices you mentioned? Beyond the base file, is there any exposure path I'm missing? Even with the guard, a Category-2 slice still keeps its base instant on the failed t2 (only its visibility is corrected). Is your preference for patch 1 mainly about moving the anchor onto a valid committed instant, so this "identity" issue is avoided at the root? If Category 2 is also handled by re-anchoring at construction time (patch 1), then isFileSliceCommitted wouldn't need to change at all — is that the route you'd rather take? Switched the fix to **patch 1 (re-anchor at file-group construction)**. ### Why not patch 2 The approach I originally proposed still has issues: it leaves the slice keyed on an **uncommitted** instant and only carves out an exception in the visibility gate to surface it — treating the symptom, not the cause. It also misses the read paths that don't trim (e.g. the legacy Hive/MR incremental realtime read), and any newly added view API that forgets to trim would re-introduce the leak. ### Why patch 1 is better 1. **Fixes the root cause** — the earliest *completed* log establishes the initial slice, so the slice's base instant is itself committed; an earlier uncommitted log degrades to a pending log attached to that slice. 2. **Version-agnostic** — patch 2's rescue branch only kicks in on v8+, so the semantics diverge across versions; patch 1 is driven by completion time and behaves the same on v6 and v8+. 3. **Localized change** — everything lives at file-group construction. The sorting contract is pulled back inside: `addLogFiles` owns the sort + re-anchoring, and `addLogFile` becomes `private`. Branch updated accordingly. -- 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]
