JingsongLi commented on code in PR #10897:
URL: https://github.com/apache/paimon/pull/10897#discussion_r4435419296
##########
paimon-core/src/main/java/org/apache/paimon/table/AbstractFileStoreTable.java:
##########
@@ -507,7 +516,131 @@ public ExpireSnapshots newExpireSnapshots() {
changelogManager(),
store().newSnapshotDeletion(),
store().newTagManager(),
- store().options().scanManifestParallelism());
+ store().options().scanManifestParallelism(),
+ newOtherBranchesLiveFiles());
+ }
+
+ /**
+ * Builds a collector for data files referenced by branches other than the
one being expired.
+ *
+ * <p>Every branch shares the same physical data directory, so expiring
snapshots on one branch
+ * must not delete files another branch (for example main) still
references. Returns {@code
+ * null} when there is no other branch, keeping the common single-branch
path untouched.
+ */
+ @Nullable
+ private ExpireSnapshotsImpl.OtherBranchesLiveFiles
newOtherBranchesLiveFiles() {
+ String currentBranch = BranchManager.normalizeBranch(currentBranch());
+ List<String> otherBranches;
+ try {
+ List<String> allBranches = new
ArrayList<>(branchManager().branches());
Review Comment:
[P1] Discover branches for each expiration, not only when constructing the
expirer. `newExpireRunnable()` creates this object once and reuses it for every
commit. If a streaming writer starts before a branch is created, this code
returns null (or captures a list omitting that branch), so that branch is never
protected by the running writer. Reproduced on an append table: write snapshot
1, create a tag, cache `newExpireSnapshots()`, create a branch from the tag,
remove the main tag, append snapshot 2, expire to one snapshot; reading the
branch fails with FileNotFoundException for its manifest list. Constructing the
expirer after branch creation passes the identical control. Refresh the branch
inventory within each expiry batch, including the initially single-branch case.
##########
paimon-core/src/main/java/org/apache/paimon/table/AbstractFileStoreTable.java:
##########
@@ -507,7 +516,131 @@ public ExpireSnapshots newExpireSnapshots() {
changelogManager(),
store().newSnapshotDeletion(),
store().newTagManager(),
- store().options().scanManifestParallelism());
+ store().options().scanManifestParallelism(),
+ newOtherBranchesLiveFiles());
+ }
+
+ /**
+ * Builds a collector for data files referenced by branches other than the
one being expired.
+ *
+ * <p>Every branch shares the same physical data directory, so expiring
snapshots on one branch
+ * must not delete files another branch (for example main) still
references. Returns {@code
+ * null} when there is no other branch, keeping the common single-branch
path untouched.
+ */
+ @Nullable
+ private ExpireSnapshotsImpl.OtherBranchesLiveFiles
newOtherBranchesLiveFiles() {
+ String currentBranch = BranchManager.normalizeBranch(currentBranch());
+ List<String> otherBranches;
+ try {
+ List<String> allBranches = new
ArrayList<>(branchManager().branches());
+ allBranches.add(Identifier.DEFAULT_MAIN_BRANCH);
+ otherBranches =
+ allBranches.stream()
+ .map(BranchManager::normalizeBranch)
+ .distinct()
+ .filter(branch -> !branch.equals(currentBranch))
+ .collect(Collectors.toList());
+ } catch (Exception e) {
+ LOG.warn(
+ "Failed to list branches before snapshot expiration. "
+ + "Cross-branch data file protection is disabled
for this run.",
+ e);
+ return null;
Review Comment:
[P1] Retain the batch when branch discovery fails. Returning null here
selects unprotected deletion, contrary to the stated fail-safe behavior;
because the expirer is reused, even a transient listing error disables
protection for its lifetime. FileSystemBranchManager.branches() can throw for a
filesystem listing failure. In an actual append-table/tag/branch fixture,
injecting that listing failure before constructing the expirer lets main
expiration delete the branch's manifest list and the branch read fails.
Propagate the failure into the existing keep-batch path (and retry discovery
next run), instead of treating an unknown branch set as no branches.
##########
paimon-core/src/main/java/org/apache/paimon/table/AbstractFileStoreTable.java:
##########
@@ -507,7 +516,131 @@ public ExpireSnapshots newExpireSnapshots() {
changelogManager(),
store().newSnapshotDeletion(),
store().newTagManager(),
- store().options().scanManifestParallelism());
+ store().options().scanManifestParallelism(),
+ newOtherBranchesLiveFiles());
Review Comment:
[P1] Apply shared-file protection to the deferred changelog cleanup too.
With `changelog-producer=none` and changelog retention greater than snapshot
retention, append-table deletion is deferred to
ExpireChangelogImpl/ChangelogDeletion, but this collector is only installed on
snapshot expiration. Reproduced using supported retention options: snapshot 1
has A and is branched; overwrite main to B, append snapshots 3/4, remove the
main tag, then expire snapshots. The branch still reads A at this point.
Expiring changelogs [1,3) then deletes its referenced manifest list and the
same branch read fails with FileNotFoundException. This also runs after
snapshot expiration in `newExpireRunnable()`. Share the cross-branch data and
metadata skipping sets with changelog expiration, and add a decoupled-lifecycle
regression case.
--
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]
