You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "amaliujia (via GitHub)" <gi...@apache.org> on 2026/08/28 03:44:22 UTC
[PR] RATIS-244. Followup: Maintain backward compatibility for cleanupOldSnapshots [ratis]
amaliujia opened a new pull request, #1569:
URL: https://github.com/apache/ratis/pull/1569
## What changes were proposed in this pull request?
Follow-up to #1320 (RATIS-244), which changed SimpleStateMachineStorage.cleanupOldSnapshots to count only snapshots with MD5 files toward the retention limit.
Meanwhile RATIS-244 maintained the backward compatibility for `findLatestSnapshot`.
That change broke backward compatibility during upgrades where old snapshots (created before MD5 support, without .md5 files) coexist with new snapshots (with MD5 files) or there is no new snapshots (with MD5 files). In those cases, cleanup could either skip deletion entirely or retain the wrong set of snapshots, because old snapshots were excluded from the retention count even though they were valid before the upgrade.
This PR restores the original retention behavior as a fallback: when fewer than numSnapshotsRetained MD5 snapshots are found, retain the newest numSnapshotsRetained snapshots regardless of whether they have MD5 files. This covers:
1. All old snapshots without MD5 — retention policy applies as it did before RATIS-244
2. Mixed old and new snapshots after upgrade — old snapshots count toward retention when there are not enough new MD5 snapshots to fill the quota
3. When enough MD5 snapshots exist, the RATIS-244 behavior is unchanged: retention is based on MD5 snapshots, and snapshots without MD5 that fall beyond that cutoff are still cleaned up.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-244
## How was this patch tested?
Unit Tests
This PR was assisted by Cursor.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2680. Maintain backward compatibility for cleanupOldSnapshots [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1569:
URL: https://github.com/apache/ratis/pull/1569#discussion_r3891965756
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -135,6 +135,14 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy snapshotRetentionPolicy)
}
}
+ // Backward compatibility: before MD5 files existed, all snapshots counted toward
+ // retention. When there are fewer MD5 snapshots than numSnapshotsRetained (e.g.
+ // all old snapshots without MD5, or old snapshots mixed with new ones after upgrade),
+ // fall back to retaining the newest numSnapshotsRetained snapshots regardless of MD5.
+ if (deleteIdx < 0 && allSnapshotFiles.size() > numSnapshotsRetained) {
+ deleteIdx = numSnapshotsRetained;
+ }
+
Review Comment:
Ack. It makes sense to differentiate the corrupted snapshots after the upgrade with legit snapshots before the snapshots (both are lack of MD5 files).
Have applied this code suggestion. Will ask @spacemonkd to review as well.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-244. Followup: Maintain backward compatibility for cleanupOldSnapshots [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1569:
URL: https://github.com/apache/ratis/pull/1569#discussion_r3882545021
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -135,6 +135,14 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy snapshotRetentionPolicy)
}
}
+ // Backward compatibility: before MD5 files existed, all snapshots counted toward
+ // retention. When there are fewer MD5 snapshots than numSnapshotsRetained (e.g.
+ // all old snapshots without MD5, or old snapshots mixed with new ones after upgrade),
+ // fall back to retaining the newest numSnapshotsRetained snapshots regardless of MD5.
+ if (deleteIdx < 0 && allSnapshotFiles.size() > numSnapshotsRetained) {
+ deleteIdx = numSnapshotsRetained;
+ }
+
Review Comment:
When snapshots are mixed (with and without md5), let's retain all snapshots with md5 and the newer snapshots without md5.
```diff
+++ b/ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java
@@ -120,11 +120,13 @@ public class SimpleStateMachineStorage implements StateMachineStorage {
final List<SingleFileSnapshotInfo> allSnapshotFiles = getSingleFileSnapshotInfos(stateMachineDir.toPath());
allSnapshotFiles.sort(Comparator.comparing(SingleFileSnapshotInfo::getIndex).reversed());
int numSnapshotsWithMd5 = 0;
+ int lastSnapshotsWithMd5 = -1;
int deleteIdx = -1;
for (int i = 0; i < allSnapshotFiles.size(); i++) {
final SingleFileSnapshotInfo snapshot = allSnapshotFiles.get(i);
if (snapshot.hasMd5()) {
+ lastSnapshotsWithMd5 = i;
if (++numSnapshotsWithMd5 == numSnapshotsRetained) {
// We have found the last snapshot with an MD5 file that needs to be retained
deleteIdx = i + 1;
@@ -135,6 +137,14 @@ public class SimpleStateMachineStorage implements StateMachineStorage {
}
}
+ // Before RATIS-244, the newest numSnapshotsRetained snapshots (with or without md5) are retained.
+ // For backward compatibility:
+ // - All snapshots without MD5 : retain the newest numSnapshotsRetained snapshots.
+ // - Snapshots mixed (with and without md5): retain all snapshots with md5 and the newer snapshots without md5.
+ if (deleteIdx < 0 && allSnapshotFiles.size() > numSnapshotsRetained) {
+ deleteIdx = Math.max(numSnapshotsRetained, lastSnapshotsWithMd5 + 1);
+ }
+
if (deleteIdx > 0) {
allSnapshotFiles.subList(deleteIdx, allSnapshotFiles.size())
.stream()
```
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2680. Maintain backward compatibility for cleanupOldSnapshots [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1569:
URL: https://github.com/apache/ratis/pull/1569
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-244. Followup: Maintain backward compatibility for cleanupOldSnapshots [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1569:
URL: https://github.com/apache/ratis/pull/1569#issuecomment-5455254794
@amaliujia , thanks for fixing this! Please file a new JIRA. It is easier to track.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-244. Followup: Maintain backward compatibility for cleanupOldSnapshots [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1569:
URL: https://github.com/apache/ratis/pull/1569#discussion_r3882545021
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -135,6 +135,14 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy snapshotRetentionPolicy)
}
}
+ // Backward compatibility: before MD5 files existed, all snapshots counted toward
+ // retention. When there are fewer MD5 snapshots than numSnapshotsRetained (e.g.
+ // all old snapshots without MD5, or old snapshots mixed with new ones after upgrade),
+ // fall back to retaining the newest numSnapshotsRetained snapshots regardless of MD5.
+ if (deleteIdx < 0 && allSnapshotFiles.size() > numSnapshotsRetained) {
+ deleteIdx = numSnapshotsRetained;
+ }
+
Review Comment:
When snapshots are mixed (with and without md5), let's retain all snapshots with md5 and the newer snapshots without md5.
```diff
+++ b/ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java
@@ -120,11 +120,13 @@ public class SimpleStateMachineStorage implements StateMachineStorage {
final List<SingleFileSnapshotInfo> allSnapshotFiles = getSingleFileSnapshotInfos(stateMachineDir.toPath());
allSnapshotFiles.sort(Comparator.comparing(SingleFileSnapshotInfo::getIndex).reversed());
int numSnapshotsWithMd5 = 0;
+ int lastSnapshotsWithMd5 = -1;
int deleteIdx = -1;
for (int i = 0; i < allSnapshotFiles.size(); i++) {
final SingleFileSnapshotInfo snapshot = allSnapshotFiles.get(i);
if (snapshot.hasMd5()) {
+ lastSnapshotsWithMd5 = i;
if (++numSnapshotsWithMd5 == numSnapshotsRetained) {
// We have found the last snapshot with an MD5 file that needs to be retained
deleteIdx = i + 1;
@@ -135,6 +137,14 @@ public class SimpleStateMachineStorage implements StateMachineStorage {
}
}
+ if (deleteIdx < 0 && allSnapshotFiles.size() > numSnapshotsRetained) {
+ // Before RATIS-244, the newest numSnapshotsRetained snapshots (with or without md5) are retained.
+ // For backward compatibility:
+ // - All snapshots without MD5 : retain the newest numSnapshotsRetained snapshots.
+ // - Snapshots mixed (with and without md5): retain all snapshots with md5 and the newer snapshots without md5.
+ deleteIdx = Math.max(numSnapshotsRetained, lastSnapshotsWithMd5 + 1);
+ }
+
if (deleteIdx > 0) {
allSnapshotFiles.subList(deleteIdx, allSnapshotFiles.size())
.stream()
```
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2680. Maintain backward compatibility for cleanupOldSnapshots [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1569:
URL: https://github.com/apache/ratis/pull/1569#discussion_r3892559619
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -135,6 +135,14 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy snapshotRetentionPolicy)
}
}
+ // Backward compatibility: before MD5 files existed, all snapshots counted toward
+ // retention. When there are fewer MD5 snapshots than numSnapshotsRetained (e.g.
+ // all old snapshots without MD5, or old snapshots mixed with new ones after upgrade),
+ // fall back to retaining the newest numSnapshotsRetained snapshots regardless of MD5.
+ if (deleteIdx < 0 && allSnapshotFiles.size() > numSnapshotsRetained) {
Review Comment:
sure. I applied by only checking `deleteIdx == -1`
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2680. Maintain backward compatibility for cleanupOldSnapshots [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #1569:
URL: https://github.com/apache/ratis/pull/1569#discussion_r3892559619
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -135,6 +135,14 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy snapshotRetentionPolicy)
}
}
+ // Backward compatibility: before MD5 files existed, all snapshots counted toward
+ // retention. When there are fewer MD5 snapshots than numSnapshotsRetained (e.g.
+ // all old snapshots without MD5, or old snapshots mixed with new ones after upgrade),
+ // fall back to retaining the newest numSnapshotsRetained snapshots regardless of MD5.
+ if (deleteIdx < 0 && allSnapshotFiles.size() > numSnapshotsRetained) {
Review Comment:
sure. I applied by only checking `deleteIdx == -1` but not `<0`
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -135,6 +135,14 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy snapshotRetentionPolicy)
}
}
+ // Backward compatibility: before MD5 files existed, all snapshots counted toward
+ // retention. When there are fewer MD5 snapshots than numSnapshotsRetained (e.g.
+ // all old snapshots without MD5, or old snapshots mixed with new ones after upgrade),
+ // fall back to retaining the newest numSnapshotsRetained snapshots regardless of MD5.
+ if (deleteIdx < 0 && allSnapshotFiles.size() > numSnapshotsRetained) {
Review Comment:
sure. I applied by only checking `deleteIdx == -1` but not `< 0`
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-244. Followup: Maintain backward compatibility for cleanupOldSnapshots [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1569:
URL: https://github.com/apache/ratis/pull/1569#discussion_r3882545021
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -135,6 +135,14 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy snapshotRetentionPolicy)
}
}
+ // Backward compatibility: before MD5 files existed, all snapshots counted toward
+ // retention. When there are fewer MD5 snapshots than numSnapshotsRetained (e.g.
+ // all old snapshots without MD5, or old snapshots mixed with new ones after upgrade),
+ // fall back to retaining the newest numSnapshotsRetained snapshots regardless of MD5.
+ if (deleteIdx < 0 && allSnapshotFiles.size() > numSnapshotsRetained) {
+ deleteIdx = numSnapshotsRetained;
+ }
+
Review Comment:
When snapshots are mixed (with and without md5), let's retain all snapshots with md5 and the newer snapshots without md5.
```
+++ b/ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java
@@ -120,11 +120,13 @@ public class SimpleStateMachineStorage implements StateMachineStorage {
final List<SingleFileSnapshotInfo> allSnapshotFiles = getSingleFileSnapshotInfos(stateMachineDir.toPath());
allSnapshotFiles.sort(Comparator.comparing(SingleFileSnapshotInfo::getIndex).reversed());
int numSnapshotsWithMd5 = 0;
+ int lastSnapshotsWithMd5 = -1;
int deleteIdx = -1;
for (int i = 0; i < allSnapshotFiles.size(); i++) {
final SingleFileSnapshotInfo snapshot = allSnapshotFiles.get(i);
if (snapshot.hasMd5()) {
+ lastSnapshotsWithMd5 = i;
if (++numSnapshotsWithMd5 == numSnapshotsRetained) {
// We have found the last snapshot with an MD5 file that needs to be retained
deleteIdx = i + 1;
@@ -135,6 +137,14 @@ public class SimpleStateMachineStorage implements StateMachineStorage {
}
}
+ // Before RATIS-244, the newest numSnapshotsRetained snapshots (with or without md5) are retained.
+ // For backward compatibility:
+ // - All snapshots without MD5 : retain the newest numSnapshotsRetained snapshots.
+ // - Snapshots mixed (with and without md5): retain all snapshots with md5 and the newer snapshots without md5.
+ if (deleteIdx < 0 && allSnapshotFiles.size() > numSnapshotsRetained) {
+ deleteIdx = Math.max(numSnapshotsRetained, lastSnapshotsWithMd5 + 1);
+ }
+
if (deleteIdx > 0) {
allSnapshotFiles.subList(deleteIdx, allSnapshotFiles.size())
.stream()
```
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-244. Followup: Maintain backward compatibility for cleanupOldSnapshots [ratis]
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on PR #1569:
URL: https://github.com/apache/ratis/pull/1569#issuecomment-5448132237
R: @spacemonkd @szetszwo
cc @peterxcli
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-244. Followup: Maintain backward compatibility for cleanupOldSnapshots [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1569:
URL: https://github.com/apache/ratis/pull/1569#issuecomment-5457464137
> out of curiosity, which scenario would make this happen? ...
Suppose there is some data corruption.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-244. Followup: Maintain backward compatibility for cleanupOldSnapshots [ratis]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #1569:
URL: https://github.com/apache/ratis/pull/1569#discussion_r3878487705
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -135,6 +135,14 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy snapshotRetentionPolicy)
}
}
+ // Backward compatibility: before MD5 files existed, all snapshots counted toward
+ // retention. When there are fewer MD5 snapshots than numSnapshotsRetained (e.g.
+ // all old snapshots without MD5, or old snapshots mixed with new ones after upgrade),
+ // fall back to retaining the newest numSnapshotsRetained snapshots regardless of MD5.
+ if (deleteIdx < 0 && allSnapshotFiles.size() > numSnapshotsRetained) {
Review Comment:
also there is a nit: we only need to do this if we havent found snapshot to delete?
```suggestion
if (deleteIdx == -1 && allSnapshotFiles.size() > numSnapshotsRetained) {
```
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -135,6 +135,14 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy snapshotRetentionPolicy)
}
}
+ // Backward compatibility: before MD5 files existed, all snapshots counted toward
+ // retention. When there are fewer MD5 snapshots than numSnapshotsRetained (e.g.
+ // all old snapshots without MD5, or old snapshots mixed with new ones after upgrade),
+ // fall back to retaining the newest numSnapshotsRetained snapshots regardless of MD5.
+ if (deleteIdx < 0 && allSnapshotFiles.size() > numSnapshotsRetained) {
Review Comment:
I see `numSnapshotsRetained` is fetched from a policy, but looks like must be the number of all snapshot files that has md5 then the logic here could be valid.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-244. Followup: Maintain backward compatibility for cleanupOldSnapshots [ratis]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #1569:
URL: https://github.com/apache/ratis/pull/1569#discussion_r3878487363
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -135,6 +135,14 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy snapshotRetentionPolicy)
}
}
+ // Backward compatibility: before MD5 files existed, all snapshots counted toward
+ // retention. When there are fewer MD5 snapshots than numSnapshotsRetained (e.g.
+ // all old snapshots without MD5, or old snapshots mixed with new ones after upgrade),
+ // fall back to retaining the newest numSnapshotsRetained snapshots regardless of MD5.
+ if (deleteIdx < 0 && allSnapshotFiles.size() > numSnapshotsRetained) {
Review Comment:
I see `numSnapshotsRetained` is fetched from a policy, but looks like must be the number of all snapshot files that has md5 then the logic here could be valid.
not sure other polcies also make this valid.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-244. Followup: Maintain backward compatibility for cleanupOldSnapshots [ratis]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #1569:
URL: https://github.com/apache/ratis/pull/1569#issuecomment-5456695826
> Consider a a corruption case:
>
> * numSnapshotsRetained = 3
> * Snapshots:
>
> ```
> snapshot.1_23
> snapshot.1_22
> snapshot.1_21
> snapshot.1_20
> snapshot.1_20.md5
> ```
out of curiosity, which scenario would make this happen? I would expect all of files that have md5 would has greater index than those doesnt because we snapshot order must be linear so files that produced with newer version must newer than older ones
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org