You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "ArnavBalyan (via GitHub)" <gi...@apache.org> on 2026/08/27 13:22:01 UTC
[PR] [core] Fix snapshot lookup for duplicate commit timestamps [paimon]
ArnavBalyan opened a new pull request, #9437:
URL: https://github.com/apache/paimon/pull/9437
### Purpose
- Multiple snapshots can have the same millisecond commit timestamp.
- Timestamp lookup stops at an arbitrary snapshot, causing time travel and rollback operations to select the wrong snapshot when multiple snapshots share the same timestamp.
- Continue searching equal timestamps to return the newest snapshot for earlier/equal lookup and the earliest snapshot for later/equal lookup.
### Tests
- UT
--
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] [core] Fix snapshot lookup for duplicate commit timestamps [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #9437:
URL: https://github.com/apache/paimon/pull/9437#discussion_r3912284758
##########
paimon-core/src/main/java/org/apache/paimon/utils/SnapshotManager.java:
##########
@@ -382,8 +382,8 @@ public long repairEarliestSnapshot(long snapshotId) {
earliest = mid + 1; // Search in the right half
finalSnapshot = snapshot;
} else {
- finalSnapshot = snapshot; // Found the exact match
- break;
+ finalSnapshot = snapshot;
+ earliest = mid + 1;
Review Comment:
[P2] Retry if rollback moves the upper bound after the first equal probe
Resolving `latestSnapshot` only closes the race before the binary search starts. With `0@(t-1), 1@t, 2@t, 3@t, 4@(t+1)`, `earlierOrEqualTimeMills(t)` first records snapshot 1. If `RollbackHelper` retains snapshot 1 at that point, it commits `LATEST=1` and deletes snapshots 4 through 2. This branch advances `earliest` to 2, so the next `snapshot(2)` throws even though snapshot 1 is still the correct result. I reproduced this deterministically by triggering rollback immediately after the first equal probe.
Please read probes through `tryGetSnapshot` and, when a post-match probe disappears, refresh the live upper bound and restart with a bounded retry. A regression test should trigger rollback after the first equal midpoint has been read.
##########
paimon-core/src/main/java/org/apache/paimon/utils/SnapshotManager.java:
##########
@@ -415,8 +415,8 @@ public long repairEarliestSnapshot(long snapshotId) {
} else if (commitTime < timestampMills) {
earliest = mid + 1; // Search in the right half
} else {
- finalSnapshot = snapshot; // Found the exact match
- break;
+ finalSnapshot = snapshot;
+ latest = mid - 1;
Review Comment:
[P2] Retry if expiration moves the lower bound after the first equal probe
Resolving the initial earliest snapshot does not cover expiration that starts during the binary search. With `0@(t-1), 1@t, 2@t, 3@t, 4@(t+1)`, `laterOrEqualTimeMills(t)` first records snapshot 2. If expiration then retains snapshot 2 and deletes snapshots 0 and 1, this branch moves `latest` to 1 and the next `snapshot(1)` throws, although snapshot 2 is the correct earliest surviving result. I reproduced this deterministically by expiring the prefix immediately after the first equal probe.
Please handle a missing post-match probe by refreshing the live lower bound and restarting with a bounded retry, and add a regression test where expiration runs after the first equal midpoint has been read.
--
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] [core] Fix snapshot lookup for duplicate commit timestamps [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #9437:
URL: https://github.com/apache/paimon/pull/9437#discussion_r3912284758
##########
paimon-core/src/main/java/org/apache/paimon/utils/SnapshotManager.java:
##########
@@ -382,8 +382,8 @@ public long repairEarliestSnapshot(long snapshotId) {
earliest = mid + 1; // Search in the right half
finalSnapshot = snapshot;
} else {
- finalSnapshot = snapshot; // Found the exact match
- break;
+ finalSnapshot = snapshot;
+ earliest = mid + 1;
Review Comment:
[P2] Retry if rollback moves the upper bound after the first equal probe\n\nResolving only closes the race before the binary search starts. With , first records snapshot 1. If retains snapshot 1 at that point, it commits and deletes snapshots 4 through 2. This branch advances to 2, so the next throws even though snapshot 1 is still the correct result. I reproduced this deterministically by triggering rollback immediately after the first equal probe.\n\nPlease read probes through and, when a post-match probe disappears, refresh the live upper bound and restart with a bounded retry. A regression test should trigger rollback after the first equal midpoint has been read.
--
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] [core] Fix snapshot lookup for duplicate commit timestamps [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #9437:
URL: https://github.com/apache/paimon/pull/9437#discussion_r3878852125
##########
paimon-core/src/main/java/org/apache/paimon/utils/SnapshotManager.java:
##########
@@ -415,8 +415,8 @@ public long repairEarliestSnapshot(long snapshotId) {
} else if (commitTime < timestampMills) {
earliest = mid + 1; // Search in the right half
} else {
- finalSnapshot = snapshot; // Found the exact match
- break;
+ finalSnapshot = snapshot;
+ latest = mid - 1;
Review Comment:
[P2] Refresh the lower bound before searching past an exact match
`earliest` can become stale when expiration runs between `earliestSnapshotId()` and the binary search. With snapshots 0/1/2 sharing this timestamp, expiration can delete snapshot 0 after the ID is read; the search first hits snapshot 1, then this new continuation probes snapshot 0 and fails even though snapshot 1 is the earliest surviving match. The old `break` returned snapshot 1.
Please obtain the lower bound through `earliestSnapshot(latest)`, which already retries concurrent earliest deletion, return it directly when it satisfies the query, and cover this race in the 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] [core] Fix snapshot lookup for duplicate commit timestamps [paimon]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on PR #9437:
URL: https://github.com/apache/paimon/pull/9437#issuecomment-5468278132
Thanks @JingsongLi! addressed both comments. The lookup now resolves live snapshot and returns it directly when eligible thanks
--
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] [core] Fix snapshot lookup for duplicate commit timestamps [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #9437:
URL: https://github.com/apache/paimon/pull/9437#discussion_r3912295256
##########
paimon-core/src/test/java/org/apache/paimon/utils/SnapshotManagerTest.java:
##########
@@ -423,6 +423,64 @@ public void testLaterOrEqualTimeMills() throws IOException {
assertThat(snapshotManager.laterOrEqualTimeMills(millis + 10001)).isNull();
}
+ @Test
+ public void testEarlierOrEqualTimeMillsWithDuplicateCommitTimes() throws IOException {
+ long millis = 1684726826L;
+ FileIO localFileIO = LocalFileIO.create();
+ SnapshotManager snapshotManager =
+ newSnapshotManager(localFileIO, new Path(tempDir.toString()));
+ for (long i = 0; i < 3; i++) {
+ Snapshot snapshot = createSnapshotWithMillis(i, millis);
+ localFileIO.tryToWriteAtomic(snapshotManager.snapshotPath(i), snapshot.toJson());
+ }
+
+ assertThat(snapshotManager.earlierOrEqualTimeMills(millis).id()).isEqualTo(2);
Review Comment:
[P2] Exercise an interior duplicate-timestamp range
These duplicate-time tests give every snapshot the same timestamp, so `earlierOrEqualTimeMills` returns through the latest-snapshot shortcut and `laterOrEqualTimeMills` returns through the earliest-snapshot shortcut. Neither test executes the changed equality branches inside the binary searches. The two race tests added below also end at those shortcuts. I temporarily restored both equality branches to the old `break`, while keeping the new boundary logic, and all four tests introduced by this PR still passed.
Please add a history such as `[t-1, t, t, t, t+1]` and assert that earlier-or-equal returns the last `t` snapshot while later-or-equal returns the first one.
--
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] [core] Fix snapshot lookup for duplicate commit timestamps [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #9437:
URL: https://github.com/apache/paimon/pull/9437#discussion_r3878851053
##########
paimon-core/src/main/java/org/apache/paimon/utils/SnapshotManager.java:
##########
@@ -382,8 +382,8 @@ public long repairEarliestSnapshot(long snapshotId) {
earliest = mid + 1; // Search in the right half
finalSnapshot = snapshot;
} else {
- finalSnapshot = snapshot; // Found the exact match
- break;
+ finalSnapshot = snapshot;
+ earliest = mid + 1;
Review Comment:
[P2] Refresh the upper bound before searching past an exact match
`latest` is only an ID captured before the search. A concurrent rollback can update the latest hint and delete that snapshot after `latestSnapshotId()` returns. With snapshots 0/1/2 sharing this timestamp, if rollback removes snapshot 2 after we capture it, the first probe hits snapshot 1; this new continuation then probes snapshot 2 and throws `Snapshot file ... does not exist`, although snapshot 1 is still the correct answer. The old `break` returned snapshot 1.
Please resolve the upper boundary through the existing live-snapshot path (for example, `latestSnapshot()` plus an early boundary return) and add a rollback-race regression 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] [core] Fix snapshot lookup for duplicate commit timestamps [paimon]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #9437:
URL: https://github.com/apache/paimon/pull/9437#discussion_r4007618372
##########
paimon-core/src/test/java/org/apache/paimon/utils/SnapshotManagerTest.java:
##########
@@ -423,6 +423,64 @@ public void testLaterOrEqualTimeMills() throws IOException {
assertThat(snapshotManager.laterOrEqualTimeMills(millis + 10001)).isNull();
}
+ @Test
+ public void testEarlierOrEqualTimeMillsWithDuplicateCommitTimes() throws IOException {
+ long millis = 1684726826L;
+ FileIO localFileIO = LocalFileIO.create();
+ SnapshotManager snapshotManager =
+ newSnapshotManager(localFileIO, new Path(tempDir.toString()));
+ for (long i = 0; i < 3; i++) {
+ Snapshot snapshot = createSnapshotWithMillis(i, millis);
+ localFileIO.tryToWriteAtomic(snapshotManager.snapshotPath(i), snapshot.toJson());
+ }
+
+ assertThat(snapshotManager.earlierOrEqualTimeMills(millis).id()).isEqualTo(2);
Review Comment:
Thanks @JingsongLi! updated both paths to restart with refreshed snapshot boundaries when concurrent rollback or expiration removes a snapshot during the search
--
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] [core] Fix snapshot lookup for duplicate commit timestamps [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #9437:
URL: https://github.com/apache/paimon/pull/9437#discussion_r4051988881
##########
paimon-core/src/main/java/org/apache/paimon/utils/SnapshotManager.java:
##########
@@ -147,13 +152,44 @@ public Snapshot tryGetSnapshot(long snapshotId) throws FileNotFoundException {
public boolean snapshotExists(long snapshotId) {
Path path = snapshotPath(snapshotId);
- try {
- return fileIO.exists(path);
- } catch (IOException e) {
- throw new RuntimeException(
- "Failed to determine if snapshot #" + snapshotId + " exists in path " + path,
- e);
+ IOException failure = null;
+ for (int attempt = 1; attempt <= SNAPSHOT_EXISTS_MAX_ATTEMPTS; attempt++) {
Review Comment:
[P2] Split the unrelated `snapshotExists` retry into its own PR
This change also rewrites `snapshotExists` to retry `fileIO.exists` three times with a 1s blocking sleep (`SNAPSHOT_EXISTS_MAX_ATTEMPTS`, `SNAPSHOT_EXISTS_RETRY_INTERVAL_MILLIS`). It is unrelated to the duplicate-timestamp lookup, is not mentioned in the PR description, and has no test. It applies to a method with a dozen-plus production call sites, including commit paths (`StrictModeChecker`, `IcebergCommitCallback`), streaming poll loops (`NextSnapshotFetcher`, `AppendCompactCoordinator`, `TagAutoCreation`) and branch/tag maintenance (`FileSystemBranchManager`, `TagManager`).
**Trigger**: any caller where `fileIO.exists` throws persistently (permission or short-read errors, an object store under throttling).
**Impact**: every `snapshotExists` call then blocks up to 2s, logs two WARNs and issues 3x the number of `exists` requests before failing with the same `RuntimeException` as before. So a focused race-fix PR silently changes failure latency and request volume for commit/streaming paths. Note that the lookup fix retries because it must re-resolve concurrent rollback/expiration; this retry only re-issues an identical `exists`, so its benefit is limited to transient FS errors, which one would not expect on the same path the PR is about.
**Fix**: keep this PR to the duplicate-timestamp lookup and move the retry to a separate PR with its own rationale and test (e.g. a `FileIO` that throws once and then succeeds). If it is kept here, please add a test and consider a shorter interval (the existing `tryFromPath` retry uses 200ms).
--
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