You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2022/11/26 00:11:32 UTC
[GitHub] [ratis] szetszwo commented on a diff in pull request #790: RATIS-1752. Clean md5 file created by old ratis version.
szetszwo commented on code in PR #790:
URL: https://github.com/apache/ratis/pull/790#discussion_r1032710109
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -58,6 +60,8 @@ public class SimpleStateMachineStorage implements StateMachineStorage {
/** snapshot.term_index */
public static final Pattern SNAPSHOT_REGEX =
Pattern.compile(SNAPSHOT_FILE_PREFIX + "\\.(\\d+)_(\\d+)");
+ public static final Pattern SNAPSHOT_MD5_REGEX =
+ Pattern.compile(SNAPSHOT_FILE_PREFIX + "\\.(\\d+)_(\\d+)" + MD5_SUFFIX);
Review Comment:
We map also add a filter for later use.
```java
private static final DirectoryStream.Filter<Path> SNAPSHOT_MD5_FILTER
= entry -> Optional.ofNullable(entry.getFileName())
.map(Path::toString)
.map(SNAPSHOT_MD5_REGEX::matcher)
.filter(Matcher::matches)
.isPresent();
```
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -121,6 +126,26 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy snapshotRetentionPolicy)
FileUtils.deleteFileQuietly(md5file);
}
}
+ if (snapshotToBeCleaned.isEmpty()) {
+ return;
+ }
+ // clean up old md5 file created by old ratis version
+ long cleanedSnapshotMaxIndex =
+ snapshotToBeCleaned.get(snapshotToBeCleaned.size() - 1).getIndex();
+ try (DirectoryStream<Path> stream = Files.newDirectoryStream(stateMachineDir.toPath())) {
+ for (Path path : stream) {
+ final Path md5file = path.getFileName();
+ if (md5file != null) {
+ final Matcher matcher = SNAPSHOT_MD5_REGEX.matcher(md5file.toString());
+ if (matcher.matches()) {
+ final long index = Long.parseLong(matcher.group(2));
+ if (index < cleanedSnapshotMaxIndex){
+ FileUtils.deleteFileQuietly(path.toFile());
Review Comment:
Let's change `FileUtils` so that we don't have to covert the path to a file. BTW, the javadoc for deleteFileQuietly is incorrect. Could you fix it as below?
```java
/** The same as passing f.toPath() to {@link #deletePathQuietly(Path)}. */
static boolean deleteFileQuietly(File f) {
return deletePathQuietly(f.toPath());
}
/**
* Delete the given path quietly.
* Only print a debug message in case that there is an exception,
*/
static boolean deletePathQuietly(Path p) {
try {
delete(p);
return true;
} catch (Exception ex) {
LOG.debug("Failed to delete " + p.toAbsolutePath(), ex);
return false;
}
}
```
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -109,8 +113,9 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy snapshotRetentionPolicy)
if (allSnapshotFiles.size() > snapshotRetentionPolicy.getNumSnapshotsRetained()) {
allSnapshotFiles.sort(Comparator.comparing(SingleFileSnapshotInfo::getIndex).reversed());
- List<File> snapshotFilesToBeCleaned = allSnapshotFiles.subList(
- snapshotRetentionPolicy.getNumSnapshotsRetained(), allSnapshotFiles.size()).stream()
+ List<SingleFileSnapshotInfo> snapshotToBeCleaned = allSnapshotFiles.subList(
+ snapshotRetentionPolicy.getNumSnapshotsRetained(), allSnapshotFiles.size());
+ List<File> snapshotFilesToBeCleaned = snapshotToBeCleaned.stream()
Review Comment:
Since we will check if a snapshot file exists before deleting the md5, we don't need the index and the code can be simplified as below:
```java
allSnapshotFiles.subList(numSnapshotsRetained, allSnapshotFiles.size())
.stream()
.map(SingleFileSnapshotInfo::getFile)
.map(FileInfo::getPath)
.forEach(snapshotPath -> {
LOG.info("Deleting old snapshot at {}", snapshotPath.toAbsolutePath());
final boolean deleted = FileUtils.deletePathQuietly(snapshotPath);
if (deleted) {
final File md5file = MD5FileUtil.getDigestFileForFile(snapshotPath.toFile());
FileUtils.deleteFileQuietly(md5file);
}
});
```
##########
ratis-server/src/main/java/org/apache/ratis/statemachine/impl/SimpleStateMachineStorage.java:
##########
@@ -121,6 +126,26 @@ public void cleanupOldSnapshots(SnapshotRetentionPolicy snapshotRetentionPolicy)
FileUtils.deleteFileQuietly(md5file);
}
}
+ if (snapshotToBeCleaned.isEmpty()) {
+ return;
+ }
+ // clean up old md5 file created by old ratis version
+ long cleanedSnapshotMaxIndex =
+ snapshotToBeCleaned.get(snapshotToBeCleaned.size() - 1).getIndex();
+ try (DirectoryStream<Path> stream = Files.newDirectoryStream(stateMachineDir.toPath())) {
+ for (Path path : stream) {
+ final Path md5file = path.getFileName();
+ if (md5file != null) {
+ final Matcher matcher = SNAPSHOT_MD5_REGEX.matcher(md5file.toString());
+ if (matcher.matches()) {
+ final long index = Long.parseLong(matcher.group(2));
+ if (index < cleanedSnapshotMaxIndex){
+ FileUtils.deleteFileQuietly(path.toFile());
+ }
+ }
+ }
+ }
+ }
Review Comment:
Let's clean up the md5 files if the corresponding snapshot file does not exist.
```java
// clean up the md5 files if the corresponding snapshot file does not exist
try (DirectoryStream<Path> stream = Files.newDirectoryStream(stateMachineDir.toPath(), SNAPSHOT_MD5_FILTER)) {
for (Path md5path : stream) {
final String md5file = md5path.getFileName().toString();
final File snapshotFile = new File(md5path.getParent().toFile(),
md5file.substring(0, md5file.length() - MD5FileUtil.MD5_SUFFIX.length()));
if (Files.notExists(snapshotFile.toPath())) {
FileUtils.deletePathQuietly(md5path);
}
}
}
```
--
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