zhaoyudi-creator commented on issue #19774:
URL: https://github.com/apache/hudi/issues/19774#issuecomment-5454432139
@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?
--
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]
