You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "mvanhorn (via GitHub)" <gi...@apache.org> on 2026/10/17 06:53:55 UTC
[PR] protect tags during snapshot expiration [paimon]
mvanhorn opened a new pull request, #10625:
URL: https://github.com/apache/paimon/pull/10625
### Purpose
A tag created while snapshots are expiring can return success and then fail to read. Time travel through that tag hits a missing file, because manifest and data files it still references were deleted after the tag write. ExpireSnapshotsImpl records tagged snapshots once and reuses that fixed list for data-file and manifest cleanup, deleting snapshot metadata only afterward. TagManager.createTag writes the tag on its own, with no shared lock or later recheck, so a tag committed after that collection is left out of the skip set. A snapshot-existence check around the write still passes, because the snapshot file is removed only after those referenced files are already gone.
Each table path and branch has one reentrant lock, stored in a static map and shared by every TagManager for that key. ExpireSnapshotsImpl holds it from collectTaggedSnapshots through snapshot deletion, and TagManager.createTag holds it across the existence check, ensureSnapshotRetained, and the tag metadata write. ensureSnapshotRetained allows the write when the snapshot still exists or an existing tag already retains that snapshot id; otherwise it throws SnapshotNotExistException and writes no tag. Both paths release the lock in a finally block, including when expiration or the tag write fails.
Fixes #9220
### Tests
mvn -B -ntp -pl paimon-core -am test -Dtest=ExpireSnapshotsTest -Dsurefire.failIfNoSpecifiedTests=false finished with ExpireSnapshotsTest at 41 tests, 0 failures, 0 errors, and 0 skipped.
The tests in `paimon-core/src/test/java/org/apache/paimon/operation/ExpireSnapshotsTest.java` fail without the fix in the paimon ExpireSnapshotsImpl change (checked by reverting the source changes).
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] protect tags during snapshot expiration [paimon]
Posted by "mvanhorn (via GitHub)" <gi...@apache.org>.
mvanhorn commented on PR #10625:
URL: https://github.com/apache/paimon/pull/10625#issuecomment-6297064966
Thanks for the two interleavings. Both are fixed in b995317, which also merges in current master.
- Each expiration run now writes its own marker under expire-marker/<snapshotId>/<runId> and deletes only that one, in the cleanup path too. A creator rolls its tag back while any run's marker is still there, so in your three-JVM case B can no longer clear A's fence.
- createTag now deletes the tag it just published when the post-write check fails for any reason, IOException included, and rethrows the original error, so a retry with ignoreIfExists=true does not find a leftover tag.
New tests: testExpireKeepsAnotherExpirerFence and testCreateTagRollsBackWhenMarkerCheckFails. ExpireSnapshotsTest, TagAutoManagerTest, TagManagerTest and TagTest pass on JDK 11 (69 tests), and spotless and checkstyle pass on paimon-core.
One limit: an expirer that crashes before its cleanup leaves its marker behind, and that marker keeps fencing the snapshot until it is removed.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] protect tags during snapshot expiration [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #10625:
URL: https://github.com/apache/paimon/pull/10625#discussion_r4313658468
##########
paimon-core/src/main/java/org/apache/paimon/table/ExpireSnapshotsImpl.java:
##########
@@ -217,62 +251,105 @@ private int innerExpireUntil(long earliestId, long endExclusiveId)
// tags to create data file skipper
List<Snapshot> taggedSnapshots = collectTaggedSnapshots();
+ Runnable collectedTags = afterTagCollection;
+ if (collectedTags != null) {
+ collectedTags.run();
+ }
+ // Another JVM can create a tag without this process's lock. Mark untagged snapshots, scan
+ // tags again, and keep any snapshot that published a tag before the marker. Snapshot files
+ // are still deleted at the end, same as before.
+ List<Long> marked = new ArrayList<>();
+ try {
+ retainTagsCreatedDuringExpiration(snapshotsExcludingEnd, taggedSnapshots, marked);
- // delete merge tree files
- // deleted merge tree files in a snapshot are not used by the next snapshot, so the range of
- // id should be (beginInclusiveId, endExclusiveId]
- cleanDataFiles(snapshotsIncludingEnd, taggedSnapshots, beginInclusiveId);
+ // delete merge tree files
+ // deleted merge tree files in a snapshot are not used by the next snapshot, so the
+ // range of id should be (beginInclusiveId, endExclusiveId]
+ cleanDataFiles(snapshotsIncludingEnd, taggedSnapshots, beginInclusiveId);
- // delete changelog files
- if (!expireConfig.isChangelogDecoupled()) {
- cleanChangelogFiles(snapshotsExcludingEnd);
- }
+ // delete changelog files
+ if (!expireConfig.isChangelogDecoupled()) {
+ cleanChangelogFiles(snapshotsExcludingEnd);
+ }
- // data files and changelog files in bucket directories has been deleted
- // then delete changed bucket directories if they are empty
- snapshotDeletion.cleanEmptyDirectories();
+ // data files and changelog files in bucket directories has been deleted
+ // then delete changed bucket directories if they are empty
+ snapshotDeletion.cleanEmptyDirectories();
- // delete manifests and indexFiles
- List<Snapshot> skippingSnapshots =
- findSkippingTags(taggedSnapshots, beginInclusiveId, endExclusiveId);
+ // delete manifests and indexFiles
+ List<Snapshot> skippingSnapshots =
+ findSkippingTags(taggedSnapshots, beginInclusiveId, endExclusiveId);
- Snapshot lastSnapshot = snapshotsIncludingEnd.get(snapshotsIncludingEnd.size() - 1);
- if (lastSnapshot.id() != endExclusiveId) {
- // the end exclusive snapshot is gone
- // there is no need to proceed
- return 0;
- }
- skippingSnapshots.add(lastSnapshot);
+ Snapshot lastSnapshot = snapshotsIncludingEnd.get(snapshotsIncludingEnd.size() - 1);
+ if (lastSnapshot.id() != endExclusiveId) {
+ // the end exclusive snapshot is gone
+ // there is no need to proceed
+ return 0;
+ }
+ skippingSnapshots.add(lastSnapshot);
- Set<String> skippingSet = null;
- try {
- Set<String> builtSkippingSet = ConcurrentHashMap.newKeySet();
- builtSkippingSet.addAll(snapshotDeletion.manifestSkippingSet(skippingSnapshots));
- skippingSet = builtSkippingSet;
- } catch (Exception e) {
- LOG.info("Skip cleaning manifest files due to failed to build skipping set.", e);
- }
- if (skippingSet != null) {
- snapshotDeletion.executeAll(
- collectManifestDeletionTasks(snapshotsExcludingEnd, skippingSet));
- }
+ Set<String> skippingSet = null;
+ try {
+ Set<String> builtSkippingSet = ConcurrentHashMap.newKeySet();
+ builtSkippingSet.addAll(snapshotDeletion.manifestSkippingSet(skippingSnapshots));
+ skippingSet = builtSkippingSet;
+ } catch (Exception e) {
+ LOG.info("Skip cleaning manifest files due to failed to build skipping set.", e);
+ }
+ if (skippingSet != null) {
+ snapshotDeletion.executeAll(
+ collectManifestDeletionTasks(snapshotsExcludingEnd, skippingSet));
+ }
- // delete snapshot file finally
- for (Snapshot snapshot : snapshotsExcludingEnd) {
- if (expireConfig.isChangelogDecoupled()) {
- commitChangelog(new Changelog(snapshot));
+ // delete snapshot file finally
+ for (Snapshot snapshot : snapshotsExcludingEnd) {
+ if (expireConfig.isChangelogDecoupled()) {
+ commitChangelog(new Changelog(snapshot));
+ }
+ snapshotManager.deleteSnapshot(snapshot.id());
+ }
+
+ writeEarliestHint(endExclusiveId);
+ long duration = currentTimeMillis.get() - startTime;
+ LOG.info(
+ "Finished expire snapshots, duration {} ms, range is [{}, {})",
+ duration,
+ beginInclusiveId,
+ endExclusiveId);
+ return snapshotsExcludingEnd.size();
+ } finally {
+ for (Long snapshotId : marked) {
+ tagManager.deleteExpirationMarker(snapshotId);
}
- snapshotManager.deleteSnapshot(snapshot.id());
}
+ }
- writeEarliestHint(endExclusiveId);
- long duration = currentTimeMillis.get() - startTime;
- LOG.info(
- "Finished expire snapshots, duration {} ms, range is [{}, {})",
- duration,
- beginInclusiveId,
- endExclusiveId);
- return snapshotsExcludingEnd.size();
+ private void retainTagsCreatedDuringExpiration(
+ List<Snapshot> expiring, List<Snapshot> taggedSnapshots, List<Long> marked)
+ throws ExecutionException, InterruptedException {
+ Set<Long> taggedIds = new HashSet<>();
+ for (Snapshot tagged : taggedSnapshots) {
+ taggedIds.add(tagged.id());
+ }
+ for (Snapshot snapshot : expiring) {
+ if (taggedIds.contains(snapshot.id())) {
+ tagManager.deleteExpirationMarker(snapshot.id());
Review Comment:
[P1] Preserve fences owned by other active expirations
This deletes a marker that may belong to another still-running expirer. I reproduced this with three independent JDK 8 JVMs: A marks snapshot 2 and pauses after its final tag scan; C publishes a tag for 2 and pauses before its post-write check; B sees C's tag, deletes A's marker here, preserves the tag's files, and successfully expires [1,3). C then sees no marker and surviving manifests, so tag creation succeeds. A resumes with its earlier tag set and deletes those manifests, leaving the successfully created tag unreadable. Both expirations returned successfully, with no injected failure. Automatic writer expiration can overlap an independent Spark/Flink expire_snapshots call. Please serialize expirers across processes or give each run its own fence ownership, keeping creators fenced until every expirer that missed the tag has finished. The deletion at line 345 and cleanup of borrowed markers in finally also need the same ownership guarantee.
##########
paimon-core/src/main/java/org/apache/paimon/utils/TagManager.java:
##########
@@ -181,6 +317,15 @@ private void createOrReplaceTag(
e);
}
+ if (rollbackIfSnapshotExpired) {
+ try {
+ ensureSnapshotRetained(snapshot, tagName, true);
+ } catch (SnapshotNotExistException e) {
Review Comment:
[P1] Roll back a published tag when retention validation fails with I/O
Only SnapshotNotExistException removes the tag written at line 310. If the new post-write marker/manifest existence check throws IOException, ensureSnapshotRetained wraps it as UncheckedIOException and the published tag survives without completing the retention handshake. I reproduced this with a creator in a separate JVM started after the expirer’s final tag scan: inject one transient IOException on exists(expire-marker/2), let expiration finish, then retry createTag(..., ignoreIfExists=true). The first call fails but leaves tagExists=true; expiration deletes snapshot 2's manifests; the retry returns successfully because the tag already exists; reading that tag fails with a missing manifest-list. Auto/batch tag creators use ignoreIfExists=true. Please clean up this call's newly published tag on every failed post-write retention validation (preserving the original error), so a retry cannot acknowledge an unvalidated dangling tag.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] protect tags during snapshot expiration [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi closed pull request #10625: protect tags during snapshot expiration
URL: https://github.com/apache/paimon/pull/10625
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] protect tags during snapshot expiration [paimon]
Posted by "mvanhorn (via GitHub)" <gi...@apache.org>.
mvanhorn commented on code in PR #10625:
URL: https://github.com/apache/paimon/pull/10625#discussion_r4307978113
##########
paimon-core/src/main/java/org/apache/paimon/utils/TagManager.java:
##########
@@ -64,10 +66,18 @@ public class TagManager {
public static final String TAG_PREFIX = "tag-";
+ /**
+ * One reentrant lock per table and branch, shared by every {@link TagManager} for that key so
+ * tag creation and snapshot expiration cannot interleave.
+ */
+ private static final ConcurrentHashMap<String, ReentrantLock> COORDINATION_LOCKS =
Review Comment:
Reworked in 9fdbd05 so tag creation and expiration coordinate through the file system instead of an in-JVM lock.
- Expiration writes an expire marker before its final tag scan.
- After writing its tag, a creator checks that the snapshot or manifest list files still exist and that no expire marker covers the snapshot, and rolls the tag back otherwise. A tag written by another in-flight creator no longer counts as protection.
- Snapshot files are still deleted at the end, as before.
Tests: testSeparateJvmTagCreatedDuringExpirationKeepsFiles creates a tag from a child JVM while expiration runs, and testConcurrentCreatorsAfterFinalTagScanRollBack runs two child-JVM creators after the final scan and checks both roll back. The TagAutoManagerTest TTL fixture is repaired too. The tag, expire and snapshot tests in paimon-core pass on JDK 11.
One case this does not cover: two expirations running at once on the same table can share a marker. I kept that out of scope since a table normally has one expirer, but can add it if you want.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] protect tags during snapshot expiration [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #10625:
URL: https://github.com/apache/paimon/pull/10625#discussion_r4306252564
##########
paimon-core/src/main/java/org/apache/paimon/utils/TagManager.java:
##########
@@ -64,10 +66,18 @@ public class TagManager {
public static final String TAG_PREFIX = "tag-";
+ /**
+ * One reentrant lock per table and branch, shared by every {@link TagManager} for that key so
+ * tag creation and snapshot expiration cannot interleave.
+ */
+ private static final ConcurrentHashMap<String, ReentrantLock> COORDINATION_LOCKS =
Review Comment:
[P1] Coordinate tag creation and expiration across JVMs
This static map only shares locks within one JVM/classloader. A continuously running writer expires snapshots in its JVM, while the documented standalone `flink run ... create_tag` action or a separate Spark/Java client creates tags in another JVM. Those operations take independent locks, and `ensureSnapshotRetained` still succeeds because the snapshot metadata is deleted after its data/manifests.
I reproduced this on this head using the existing expiration test fixture: pause expiration via `setAfterTagCollection`, create a tag for snapshot #2 in a separate Java process, then resume expiration. The child reports success; afterward `tagExists=true`, `snapshotExists=false`, `deltaManifestExists=false`, and reading the tag fails with `Failed to read manifest-list-...`. Thus the PR still permits the data-retention failure it claims to fix in #9220. Please use coordination/GC protection shared by all participating processes and add a deterministic independent-JVM regression test; snapshot-existence checks alone cannot close this window.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] protect tags during snapshot expiration [paimon]
Posted by "mvanhorn (via GitHub)" <gi...@apache.org>.
mvanhorn commented on code in PR #10625:
URL: https://github.com/apache/paimon/pull/10625#discussion_r4371517772
##########
paimon-core/src/main/java/org/apache/paimon/utils/TagManager.java:
##########
@@ -132,11 +161,127 @@ public void createTag(
List<TagCallback> callbacks,
boolean ignoreIfExists) {
checkArgument(!StringUtils.isNullOrWhitespaceOnly(tagName), "Tag name shouldn't be blank.");
- if (tagExists(tagName)) {
- checkArgument(ignoreIfExists, "Tag '%s' already exists.", tagName);
+ // The existence check and metadata write must not race with snapshot expiration, which
+ // deletes files based on a tag set collected under the same lock.
+ ReentrantLock coordinationLock = coordinationLock();
+ coordinationLock.lock();
+ try {
+ if (tagExists(tagName)) {
+ checkArgument(ignoreIfExists, "Tag '%s' already exists.", tagName);
+ return;
Review Comment:
Fixed in 7ba4304. createTag now writes a pending marker under tag-pending/<name>/<owner> before it publishes the tag file, and deletes it only after its post-write validation has passed or rolled the tag back. The ignoreIfExists branch no longer returns on a visible file: it waits until no pending marker is left for that name, then returns only if the tag is still there. If A rolled it back, B creates the tag itself and goes through the same validation, so in your interleaving B fails with the snapshot #2 error instead of reporting success.
New tests:
- testIgnoreIfExistsWaitsForAnotherCreatorsProvisionalTag runs your scenario: expiration paused after its final tag scan, JVM A publishes tag-same and pauses before validation, JVM B calls createTag("same", ignoreIfExists=true). B is still waiting 2 seconds later. After A is released, both exit non-zero, the tag is gone, and expiration finishes.
- testIgnoreIfExistsFailsWhileAnotherCreatorStaysPending covers a pending marker that never clears.
With the ignoreIfExists wait taken out, both tests fail. ExpireSnapshotsTest, TagManagerTest, TagTest and TagAutoManagerTest pass on JDK 11 (71 tests), and spotless and checkstyle pass on paimon-core.
One limit, the same kind as the expire markers: a creator that crashes between publishing and cleanup leaves its pending marker behind. A later ignoreIfExists call for that name then fails after 60 seconds, and the error names the directory to remove. It does not acknowledge the tag.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] protect tags during snapshot expiration [paimon]
Posted by "mvanhorn (via GitHub)" <gi...@apache.org>.
mvanhorn commented on code in PR #10625:
URL: https://github.com/apache/paimon/pull/10625#discussion_r4385518594
##########
paimon-core/src/main/java/org/apache/paimon/utils/TagManager.java:
##########
@@ -181,6 +416,15 @@ private void createOrReplaceTag(
e);
}
+ if (rollbackIfSnapshotExpired) {
+ try {
+ ensureSnapshotRetained(snapshot, tagName, true);
+ } catch (RuntimeException e) {
+ fileIO.deleteQuietly(tagPath);
Review Comment:
Fixed in 2fb7c3a. `createTag` now publishes with `fileIO.tryToWriteAtomic` (temp file plus a rename that won't overwrite) in place of `overwriteFileUtf8`. Once one creator's publication of a name succeeds, a second creator can't replace it. Its write returns false, it drops its own pending marker, and it goes through the existing path: without `ignoreIfExists` it fails with "already exists", and with it, it waits for the other creator to settle. A creator therefore only rolls back a file it wrote itself. `replaceTag` and the other overwrite callers still use `overwriteFileUtf8`.
Test: `testOverlappingCreatorsCannotOverwriteProvisionalTag` runs your schedule. B is not acknowledged with a snapshot-4 tag that later disappears. It fails on 7ba4304.
One limit: this relies on rename refusing to overwrite. That holds on HDFS and local FS. On object stores it holds only under the same catalog lock that snapshot commits already depend on. ExpireSnapshotsTest, TagManagerTest, TagTest and TagAutoManagerTest pass on JDK 11 (74 tests), and spotless and checkstyle pass on paimon-core.
<!-- r:cec9f7dcedc8.e063ebdb1cb5 -->
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] protect tags during snapshot expiration [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #10625:
URL: https://github.com/apache/paimon/pull/10625#discussion_r4346639206
##########
paimon-core/src/main/java/org/apache/paimon/utils/TagManager.java:
##########
@@ -132,11 +161,127 @@ public void createTag(
List<TagCallback> callbacks,
boolean ignoreIfExists) {
checkArgument(!StringUtils.isNullOrWhitespaceOnly(tagName), "Tag name shouldn't be blank.");
- if (tagExists(tagName)) {
- checkArgument(ignoreIfExists, "Tag '%s' already exists.", tagName);
+ // The existence check and metadata write must not race with snapshot expiration, which
+ // deletes files based on a tag set collected under the same lock.
+ ReentrantLock coordinationLock = coordinationLock();
+ coordinationLock.lock();
+ try {
+ if (tagExists(tagName)) {
+ checkArgument(ignoreIfExists, "Tag '%s' already exists.", tagName);
+ return;
Review Comment:
[P2] Do not acknowledge another creator's provisional tag as committed
The added post-write validation/rollback makes a visible tag file provisional, but this ignoreIfExists branch still treats any visible file as completed. With expiration paused after its final tag scan, JVM A publishes tag-same and pauses before validation; JVM B calls createTag for the same name with ignoreIfExists=true and successfully returns here. A then resumes, detects the active expiration marker and deletes tag-same; actual expiration finishes and B's successfully acknowledged tag does not exist. I reproduced this with real table snapshots and two creator JVMs, without I/O errors. Coordinate the existing-tag path with validation/publication ownership so it cannot succeed on an in-flight tag that can still roll back, and add a same-name cross-process retry test.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] protect tags during snapshot expiration [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #10625:
URL: https://github.com/apache/paimon/pull/10625#discussion_r4374819956
##########
paimon-core/src/main/java/org/apache/paimon/utils/TagManager.java:
##########
@@ -132,11 +172,204 @@ public void createTag(
List<TagCallback> callbacks,
boolean ignoreIfExists) {
checkArgument(!StringUtils.isNullOrWhitespaceOnly(tagName), "Tag name shouldn't be blank.");
- if (tagExists(tagName)) {
- checkArgument(ignoreIfExists, "Tag '%s' already exists.", tagName);
+ // The existence check and metadata write must not race with snapshot expiration, which
+ // deletes files based on a tag set collected under the same lock.
+ ReentrantLock coordinationLock = coordinationLock();
+ coordinationLock.lock();
+ try {
+ if (tagExists(tagName)) {
+ checkArgument(ignoreIfExists, "Tag '%s' already exists.", tagName);
+ if (awaitSettledTag(tagName)) {
+ return;
+ }
+ }
+ Runnable beforeCommit = beforeTagCommit;
+ if (beforeCommit != null) {
+ beforeCommit.run();
+ }
+ ensureSnapshotRetained(snapshot, tagName, false);
+ String owner = UUID.randomUUID().toString();
+ createPendingTagMarker(tagName, owner);
+ try {
+ createOrReplaceTag(snapshot, tagName, timeRetained, callbacks, true);
+ } finally {
+ fileIO.deleteQuietly(pendingTagMarkerPath(tagName, owner));
Review Comment:
[P1] Keep the pending fence until publication is validated or rollback is confirmed
This finally clears the pending marker even when the tag remains unvalidated. `overwriteFileUtf8` can persist the complete tag and then throw on close; its existing catch leaves that file because the outcome is unknown, so post-write retention validation never runs. Alternatively, validation can fail while `deleteQuietly(tagPath)` fails to remove the file. In both cases this finally removes the fence, and another JVM's `createTag(..., ignoreIfExists=true)` sees the tag with no pending marker and returns success.
I reproduced both cases while actual expiration was paused after its final tag scan: the retry exited successfully, then expiration completed with the tag still present but its snapshot and delta-manifest list deleted. The first case throws only after a real successful tag write; the second returns false only for the tag's deletion. Keep the pending state until publication has passed validation or rollback is confirmed. An unresolved write/delete outcome should remain fenced (as the documented crashed-creator case does), or be resolved safely before acknowledging retries.
##########
paimon-core/src/main/java/org/apache/paimon/utils/TagManager.java:
##########
@@ -181,6 +416,15 @@ private void createOrReplaceTag(
e);
}
+ if (rollbackIfSnapshotExpired) {
+ try {
+ ensureSnapshotRetained(snapshot, tagName, true);
+ } catch (RuntimeException e) {
+ fileIO.deleteQuietly(tagPath);
Review Comment:
[P1] Roll back only the publication owned by this creator
Two creator JVMs can both pass the initial absence check, so the per-name pending markers do not make this deletion owned. With expiration paused after its final scan, let B pass the absence check and wait; A publishes `same` for fenced snapshot 2 and pauses after the write. B then publishes `same` for retained snapshot 4, passes validation and returns success. When A resumes, its snapshot-2 validation fails and this deletion removes B's already acknowledged snapshot-4 tag.
An actual-expirer/two-creator JDK 8 reproduction confirms the tag points to snapshot 4 after B succeeds, then disappears when A rolls back, although snapshot 4 remains valid. Removing only this new deletion in a scratch control preserves B's tag under the identical schedule, so this is additional damage beyond the baseline's overwrite race. Please give same-name publication cross-process ownership and make rollback conditional on that ownership; a separate read-before-delete check still races with another publication. Add the overlapping-creator case alongside the existing waiter test.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] protect tags during snapshot expiration [paimon]
Posted by "mvanhorn (via GitHub)" <gi...@apache.org>.
mvanhorn commented on code in PR #10625:
URL: https://github.com/apache/paimon/pull/10625#discussion_r4385516760
##########
paimon-core/src/main/java/org/apache/paimon/utils/TagManager.java:
##########
@@ -132,11 +172,204 @@ public void createTag(
List<TagCallback> callbacks,
boolean ignoreIfExists) {
checkArgument(!StringUtils.isNullOrWhitespaceOnly(tagName), "Tag name shouldn't be blank.");
- if (tagExists(tagName)) {
- checkArgument(ignoreIfExists, "Tag '%s' already exists.", tagName);
+ // The existence check and metadata write must not race with snapshot expiration, which
+ // deletes files based on a tag set collected under the same lock.
+ ReentrantLock coordinationLock = coordinationLock();
+ coordinationLock.lock();
+ try {
+ if (tagExists(tagName)) {
+ checkArgument(ignoreIfExists, "Tag '%s' already exists.", tagName);
+ if (awaitSettledTag(tagName)) {
+ return;
+ }
+ }
+ Runnable beforeCommit = beforeTagCommit;
+ if (beforeCommit != null) {
+ beforeCommit.run();
+ }
+ ensureSnapshotRetained(snapshot, tagName, false);
+ String owner = UUID.randomUUID().toString();
+ createPendingTagMarker(tagName, owner);
+ try {
+ createOrReplaceTag(snapshot, tagName, timeRetained, callbacks, true);
+ } finally {
+ fileIO.deleteQuietly(pendingTagMarkerPath(tagName, owner));
Review Comment:
Fixed in 2fb7c3a. The pending marker is now released in only two cases: post-write validation passed, or the rollback deletion is confirmed by checking that the tag file is gone afterwards. If the write throws after persisting, validation never runs and the marker stays. If validation fails and the tag can't be deleted, the marker also stays, and the original exception propagates with the delete failure attached as suppressed. Both cases now behave like the crashed-creator case: a later `ignoreIfExists` retry waits, then fails with the error naming the pending directory. It never acknowledges the tag.
Tests: `testIgnoreIfExistsRejectsWriteFailureAfterPublication` and `testIgnoreIfExistsRejectsFailedRollbackDeletion`. Both fail on 7ba4304.
<!-- r:cec9f7dcedc8.7bb2ad7a9250 -->
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org