Stephen0421 commented on PR #9207:
URL: https://github.com/apache/paimon/pull/9207#issuecomment-5305745425
> * After `deleteQuietly` fails, the count of deleted files and bytes is
still accumulated and the path is returned, causing Local, Flink, and Spark to
falsely report success.
> * There is no lease commit between the two reachability scans and the
actual deletion; concurrent compaction might commit a snapshot that
re-references an old pack, which is subsequently deleted by GC, creating a
window for data loss.
Thanks for the review.
### 1. False success after `deleteQuietly` fails
Fixed.
Pack deletion now uses `FileIO.delete()`, and a file is counted as deleted
only when the deletion actually succeeds. The Local, Flink, and Spark
implementations all use this path.
This is covered by `testDeleteFailureIsNotReported`.
### 2. No lease between the two scans and deletion
This is a fair concern. We investigated whether a concurrent compaction
could commit after the final reachability scan and re-reference a pack already
included in the deletion set.
In the current managed-blob compaction path, reused pack references are
written to the output data file's `.blobref` based on the compaction inputs
(`compactBefore`). Therefore, at the final scan, a pack `P` falls into one of
two cases:
1. **The compaction inputs are still live.**
A retained `ADD` data file already contains a `.blobref` referencing `P`,
so the final scan marks `P` as used. A later successful compaction may reuse
`P`, but `P` is not in the deletion set.
2. **The final scan marks `P` as unused.**
No retained or current data file references `P`. A prepared compaction
that still holds a descriptor for `P` must have been created from inputs that
have since expired. Committing that compaction requires deleting its
`compactBefore` files, and conflict detection rejects the commit with `File
deletion conflicts detected`.
Equivalently, if a `compactBefore` file is still present and references `P`,
the final reachability scan treats `P` as live, so it cannot become a GC
candidate.
We added deterministic tests for both cases:
- `testSuccessfulCompactionAfterFinalMarkKeepsReusedPacks`: a compaction
based on live inputs successfully commits after the used set is frozen. Its
reused packs remain protected, while a genuine orphan is deleted.
- `testStaleCompactionCannotCommitAfterFinalMark`: a compaction based on
expired inputs attempts to commit at deletion time. The commit is rejected, and
the packs it attempted to reuse are safely deleted.
Based on the current compaction invariants, we do not believe a lease is
required for this initial version.
`older_than` is still required, but it is not a commit fence; it protects
files that were written recently. Compaction reuse does not refresh a pack's
modification time, but the two cases above ensure that a live reused pack
remains reachable and is not deleted.
If there is a compaction or rewrite path that can reference a pack outside
its `compactBefore` inputs, that would be a real gap and should be handled
separately.
--
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]
