szetszwo commented on code in PR #1320:
URL: https://github.com/apache/ratis/pull/1320#discussion_r2998468752
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SingleFileSnapshotInfo.java:
##########
@@ -28,12 +28,24 @@
* The objects of this class are immutable.
*/
public class SingleFileSnapshotInfo extends FileListSnapshotInfo {
+ private final Boolean hasMd5; // Whether the snapshot file has a
corresponding MD5 file
Review Comment:
The new field is not needed. Use `FileInfo` to determine:
```java
/** @return true iff the MD5 exists. */
public boolean hasMd5() {
return getFile().getFileDigest() != null;
}
```
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -182,24 +203,44 @@ protected File getCorruptSnapshotFile(long term, long
endIndex) {
}
static SingleFileSnapshotInfo findLatestSnapshot(Path dir) throws
IOException {
- final Iterator<SingleFileSnapshotInfo> i =
getSingleFileSnapshotInfos(dir).iterator();
- if (!i.hasNext()) {
+ final List<SingleFileSnapshotInfo> infos = getSingleFileSnapshotInfos(dir,
false);
+ if (infos.isEmpty()) {
return null;
}
+
infos.sort(Comparator.comparing(SingleFileSnapshotInfo::getIndex).reversed());
- SingleFileSnapshotInfo latest = i.next();
- for(; i.hasNext(); ) {
- final SingleFileSnapshotInfo info = i.next();
- if (info.getIndex() > latest.getIndex()) {
- latest = info;
+ // Track the latest snapshot without MD5 as fallback.
+ SingleFileSnapshotInfo fallbackWithoutMd5 = null;
+ for (SingleFileSnapshotInfo latest : infos) {
+ if (!latest.hasMd5()) {
+ if (fallbackWithoutMd5 == null) {
+ fallbackWithoutMd5 = latest;
+ }
+ continue;
+ }
+
+ final Path path = latest.getFile().getPath();
+ try {
+ final MD5Hash md5 = MD5FileUtil.readStoredMd5ForFile(path.toFile());
+ if (md5 == null) {
+ LOG.warn("Snapshot file {} has missing MD5 file.", latest);
+ continue;
+ }
+ final FileInfo info = new FileInfo(path, md5);
+ return new SingleFileSnapshotInfo(info, latest.getTerm(),
latest.getIndex(), true);
+ } catch (IOException e) {
+ LOG.warn("Failed to read MD5 for snapshot file {}, trying older
snapshots.", latest, e);
Review Comment:
This can be simplified since either all snapshots do not have MD5 or at
least one has.
```java
static SingleFileSnapshotInfo findLatestSnapshot(Path dir) throws
IOException {
final List<SingleFileSnapshotInfo> infos =
getSingleFileSnapshotInfos(dir);
if (infos.isEmpty()) {
return null;
}
infos.sort(Comparator.comparing(SingleFileSnapshotInfo::getIndex).reversed());
for (SingleFileSnapshotInfo latest : infos) {
if (latest.hasMd5()) {
return latest;
}
}
return infos.get(0); // all snapshots do not have MD5
}
```
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -82,18 +81,23 @@ public void format() throws IOException {
// 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) {
final Path filename = path.getFileName();
if (filename != null) {
final Matcher matcher = SNAPSHOT_REGEX.matcher(filename.toString());
if (matcher.matches()) {
+ final boolean hasMd5 =
MD5FileUtil.getDigestFileForFile(path.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));
Review Comment:
Let's read the md5 file here:
```java
final MD5Hash md5 =
MD5FileUtil.readStoredMd5ForFile(path.toFile());
final FileInfo fileInfo = new FileInfo(path, md5);
infos.add(new SingleFileSnapshotInfo(fileInfo, term, index));
```
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -82,18 +81,23 @@ public void format() throws IOException {
// TODO
}
- static List<SingleFileSnapshotInfo> getSingleFileSnapshotInfos(Path dir)
throws IOException {
+ static List<SingleFileSnapshotInfo> getSingleFileSnapshotInfos(Path dir,
boolean requireMd5) throws IOException {
Review Comment:
requireMd5 is always false. Let's remove it.
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -114,32 +118,49 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy
snapshotRetentionPolicy)
return;
}
- final List<SingleFileSnapshotInfo> allSnapshotFiles =
getSingleFileSnapshotInfos(stateMachineDir.toPath());
+ // Fetch all the snapshot files irrespective of whether they have an MD5
file or not
Review Comment:
Move it as javadoc of `getSingleFileSnapshotInfos(..)`.
--
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]
