szetszwo commented on code in PR #1320:
URL: https://github.com/apache/ratis/pull/1320#discussion_r2548199791
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -89,7 +89,8 @@ static List<SingleFileSnapshotInfo>
getSingleFileSnapshotInfos(Path dir) throws
final Path filename = path.getFileName();
if (filename != null) {
final Matcher matcher = SNAPSHOT_REGEX.matcher(filename.toString());
- if (matcher.matches()) {
+ // If the file doesn't have an MD5 hash it would doesn't need to be
matched as it might be corrupted
+ if (MD5FileUtil.getDigestFileForFile(filename.toFile()).exists() &&
matcher.matches()) {
Review Comment:
~Let's print a warning:~ -- it is not a good idea since it may print a lot
of messages.
Just found that the method is called also by `cleanupOldSnapshots(..)`. So,
we need more changes as below:
```diff
diff --git
a/ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java
b/ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java
index 0ca6734a0..e20ac9115 100644
---
a/ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java
+++
b/ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java
@@ -82,7 +82,7 @@ public class SimpleStateMachineStorage implements
StateMachineStorage {
// TODO
}
- static List<SingleFileSnapshotInfo> getSingleFileSnapshotInfos(Path dir)
throws IOException {
+ static List<SingleFileSnapshotInfo> getSingleFileSnapshotInfos(Path dir,
boolean requireMd5) throws IOException {
final List<SingleFileSnapshotInfo> infos = new ArrayList<>();
try (DirectoryStream<Path> stream = Files.newDirectoryStream(dir)) {
for (Path path : stream) {
@@ -90,10 +90,14 @@ public class SimpleStateMachineStorage implements
StateMachineStorage {
if (filename != null) {
final Matcher matcher =
SNAPSHOT_REGEX.matcher(filename.toString());
if (matcher.matches()) {
+ final boolean hasMd5 =
MD5FileUtil.getDigestFileForFile(filename.toFile()).exists();
+ if (requireMd5 && !hasMd5) {
+ continue;
+ }
final long term = Long.parseLong(matcher.group(1));
final long index = Long.parseLong(matcher.group(2));
final FileInfo fileInfo = new FileInfo(path, null); //No
FileDigest here.
- infos.add(new SingleFileSnapshotInfo(fileInfo, term, index));
+ infos.add(new SingleFileSnapshotInfo(fileInfo, term, index,
hasMd5));
}
}
}
@@ -114,11 +118,23 @@ public class SimpleStateMachineStorage implements
StateMachineStorage {
return;
}
- final List<SingleFileSnapshotInfo> allSnapshotFiles =
getSingleFileSnapshotInfos(stateMachineDir.toPath());
+ final List<SingleFileSnapshotInfo> allSnapshotFiles =
getSingleFileSnapshotInfos(stateMachineDir.toPath(), false);
+
allSnapshotFiles.sort(Comparator.comparing(SingleFileSnapshotInfo::getIndex).reversed());
- if (allSnapshotFiles.size() > numSnapshotsRetained) {
-
allSnapshotFiles.sort(Comparator.comparing(SingleFileSnapshotInfo::getIndex).reversed());
- allSnapshotFiles.subList(numSnapshotsRetained,
allSnapshotFiles.size())
+ int numSnapshotsWithMd5 = 0;
+ int deleteIndex = -1;
+ for(int i = 0; i < allSnapshotFiles.size(); i++) {
+ if (allSnapshotFiles.get(i).hasMd5()) {
+ numSnapshotsWithMd5++;
+ if (numSnapshotsWithMd5 == numSnapshotsRetained) {
+ deleteIndex = i+1;
+ break;
+ }
+ }
+ }
+
+ if (deleteIndex > 0) { // keep at least one snapshot
+ allSnapshotFiles.subList(deleteIndex, allSnapshotFiles.size())
.stream()
.map(SingleFileSnapshotInfo::getFile)
.map(FileInfo::getPath)
@@ -182,7 +198,7 @@ public class SimpleStateMachineStorage implements
StateMachineStorage {
}
static SingleFileSnapshotInfo findLatestSnapshot(Path dir) throws
IOException {
- final Iterator<SingleFileSnapshotInfo> i =
getSingleFileSnapshotInfos(dir).iterator();
+ final Iterator<SingleFileSnapshotInfo> i =
getSingleFileSnapshotInfos(dir, true).iterator();
if (!i.hasNext()) {
return null;
}
@@ -199,7 +215,7 @@ public class SimpleStateMachineStorage implements
StateMachineStorage {
final Path path = latest.getFile().getPath();
final MD5Hash md5 = MD5FileUtil.readStoredMd5ForFile(path.toFile());
final FileInfo info = new FileInfo(path, md5);
- return new SingleFileSnapshotInfo(info, latest.getTerm(),
latest.getIndex());
+ return new SingleFileSnapshotInfo(info, latest.getTerm(),
latest.getIndex(), true);
}
public SingleFileSnapshotInfo updateLatestSnapshot(SingleFileSnapshotInfo
info) {
diff --git
a/ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SingleFileSnapshotInfo.java
b/ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SingleFileSnapshotInfo.java
index 14d501a4a..f59d5ba77 100644
---
a/ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SingleFileSnapshotInfo.java
+++
b/ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SingleFileSnapshotInfo.java
@@ -28,12 +28,23 @@ import org.apache.ratis.server.storage.FileInfo;
* The objects of this class are immutable.
*/
public class SingleFileSnapshotInfo extends FileListSnapshotInfo {
+ private final Boolean hasMd5;
+
public SingleFileSnapshotInfo(FileInfo fileInfo, TermIndex termIndex) {
+ this(fileInfo, termIndex, null);
+ }
+
+ public SingleFileSnapshotInfo(FileInfo fileInfo, TermIndex termIndex,
Boolean hasMd5) {
super(Collections.singletonList(fileInfo), termIndex);
+ this.hasMd5 = hasMd5;
+ }
+
+ public SingleFileSnapshotInfo(FileInfo fileInfo, long term, long
endIndex, boolean hasMd5) {
+ this(fileInfo, TermIndex.valueOf(term, endIndex), hasMd5);
}
- public SingleFileSnapshotInfo(FileInfo fileInfo, long term, long
endIndex) {
- this(fileInfo, TermIndex.valueOf(term, endIndex));
+ public boolean hasMd5() {
+ return hasMd5 != null && hasMd5;
}
/** @return the file associated with the snapshot. */
```
--
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]
