Skip to content

Commit dd7dff4

Browse files
committed
[core] Check next snapshot's time for snapshotTimeRetain expiration protection
Previously, a snapshot was expired if its own timestamp exceeded snapshotTimeRetain. This could cause premature expiration when snapshotRetainMin is small. Now we check the next snapshot's timestamp instead, ensuring each snapshot is protected for the full snapshotTimeRetain duration.
1 parent 9f9b8db commit dd7dff4

2 files changed

Lines changed: 60 additions & 9 deletions

File tree

paimon-core/src/main/java/org/apache/paimon/table/ExpireSnapshotsImpl.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,10 +136,11 @@ public int expire() {
136136

137137
for (long id = min; id < maxExclusive; id++) {
138138
// Early exit the loop for 'snapshot.time-retained'
139-
// (the maximum time of snapshots to retain)
139+
// A snapshot can only be expired if its next snapshot has been alive
140+
// longer than snapshotTimeRetain, providing stronger protection
140141
try {
141-
Snapshot snapshot = snapshotManager.tryGetSnapshot(id);
142-
if (olderThanMills <= snapshot.timeMillis()) {
142+
Snapshot nextSnapshot = snapshotManager.tryGetSnapshot(id + 1);
143+
if (olderThanMills <= nextSnapshot.timeMillis()) {
143144
return expireUntil(earliest, id);
144145
}
145146
} catch (FileNotFoundException e) {

paimon-core/src/test/java/org/apache/paimon/operation/ExpireSnapshotsTest.java

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -538,13 +538,63 @@ public void testExpireWithTime() throws Exception {
538538
expire.config(builder.snapshotTimeRetain(Duration.ofMillis(1000)).build()).expire();
539539

540540
int latestSnapshotId = requireNonNull(snapshotManager.latestSnapshotId()).intValue();
541-
for (int i = 1; i <= latestSnapshotId; i++) {
542-
if (snapshotManager.snapshotExists(i)) {
543-
assertThat(snapshotManager.snapshot(i).timeMillis())
544-
.isBetween(expireMillis - 1000, expireMillis);
545-
assertSnapshot(i, allData, snapshotPositions);
546-
}
541+
// snapshots 1-4 should be expired, snapshot 5 is retained because its next
542+
// snapshot (6) is within the time window
543+
for (int i = 1; i <= 4; i++) {
544+
assertThat(snapshotManager.snapshotExists(i)).isFalse();
545+
}
546+
for (int i = 5; i <= latestSnapshotId; i++) {
547+
assertThat(snapshotManager.snapshotExists(i)).isTrue();
548+
assertSnapshot(i, allData, snapshotPositions);
549+
}
550+
551+
store.assertCleaned();
552+
}
553+
554+
@Test
555+
public void testExpireWithTimeProtectsEachSnapshot() throws Exception {
556+
// Even with a small retainMin, each snapshot should be protected by
557+
// snapshotTimeRetain: a snapshot can only be expired when its next
558+
// snapshot has been alive longer than snapshotTimeRetain.
559+
ExpireConfig.Builder builder = ExpireConfig.builder();
560+
builder.snapshotRetainMin(1)
561+
.snapshotRetainMax(Integer.MAX_VALUE)
562+
.snapshotTimeRetain(Duration.ofMillis(1000));
563+
ExpireSnapshots expire = store.newExpire(builder.build());
564+
565+
List<KeyValue> allData = new ArrayList<>();
566+
List<Integer> snapshotPositions = new ArrayList<>();
567+
568+
// create 5 snapshots quickly
569+
commit(5, allData, snapshotPositions);
570+
571+
// expire immediately - no snapshot should be expired because each
572+
// snapshot's next snapshot is still within the 1000ms time window
573+
expire.config(builder.build()).expire();
574+
575+
for (int i = 1; i <= 5; i++) {
576+
assertThat(snapshotManager.snapshotExists(i)).isTrue();
577+
assertSnapshot(i, allData, snapshotPositions);
578+
}
579+
580+
// wait for snapshotTimeRetain to pass
581+
Thread.sleep(1500);
582+
583+
// create one more snapshot so snapshot 5 has a "next"
584+
commit(1, allData, snapshotPositions);
585+
586+
// expire again - now snapshots 1-4 can be expired (their next snapshots
587+
// are older than 1000ms), but snapshot 5 is still protected because its
588+
// next snapshot (6) was just created
589+
expire.config(builder.build()).expire();
590+
591+
for (int i = 1; i <= 4; i++) {
592+
assertThat(snapshotManager.snapshotExists(i)).isFalse();
547593
}
594+
assertThat(snapshotManager.snapshotExists(5)).isTrue();
595+
assertThat(snapshotManager.snapshotExists(6)).isTrue();
596+
assertSnapshot(5, allData, snapshotPositions);
597+
assertSnapshot(6, allData, snapshotPositions);
548598

549599
store.assertCleaned();
550600
}

0 commit comments

Comments
 (0)