[GitHub] [ratis] adoroszlai commented on a change in pull request #624: RATIS-1554. Fix SnapshotManager RV_RETURN_VALUE_IGNORED_BAD_PRACTICE.

[GitBox <gi...@apache.org>]

adoroszlai commented on a change in pull request #624:
URL: https://github.com/apache/ratis/pull/624#discussion_r831959303



##########
File path: ratis-common/src/test/java/org/apache/ratis/util/FileUtilsTest.java
##########
@@ -17,25 +17,26 @@
  */
 package org.apache.ratis.util;
 
-import static org.junit.Assert.assertTrue;
+import org.junit.Assert;
+import org.junit.Test;
 
 import java.io.File;
 import java.io.IOException;
-import org.junit.Test;
 
 /** Test methods of SnapshotManager. */
 public class FileUtilsTest {
 
   @Test
   public void testRenameToCorrupt() throws IOException {
     File srcFile = File.createTempFile("snapshot.1_20", null);
-    File corruptFile = new File(srcFile.getPath() + ".corrupt");
-    if (corruptFile.exists()) {
-      FileUtils.deleteFully(corruptFile);
-    }
-    FileUtils.renameFileToCorrupt(srcFile);
-    srcFile.deleteOnExit();
-    corruptFile.deleteOnExit();
-    assertTrue(corruptFile.exists());
+    Assert.assertTrue(srcFile.exists());
+
+    final File renamed = FileUtils.move(srcFile, ".corrupt");
+    Assert.assertNotNull(renamed);
+    Assert.assertTrue(renamed.exists());
+    Assert.assertFalse(srcFile.exists());

Review comment:
       Nit: if this fails, `renamed` will not be deleted.

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java
##########
@@ -121,21 +122,63 @@ public void installSnapshot(StateMachine stateMachine,
           LOG.warn("The snapshot md5 digest {} does not match expected {}",
               digest, expectedDigest);
           // rename the temp snapshot file to .corrupt
-          FileUtils.renameFileToCorrupt(tmpSnapshotFile);
-          throw new CorruptedFileException(
-              tmpSnapshotFile, "MD5 mismatch for snapshot-" + lastIncludedIndex
-              + " installation");
+          String renameMessage;
+          try {
+            final File corruptedFile = FileUtils.move(tmpSnapshotFile, CORRUPT + StringUtils.currentDateTime());
+            renameMessage = "Renamed temporary snapshot file " + tmpSnapshotFile + " to " + corruptedFile;
+          } catch (IOException e) {
+            renameMessage = "Tried but failed to rename temporary snapshot file " + tmpSnapshotFile
+                + " to a " + CORRUPT + " file";
+            LOG.warn(renameMessage, e);
+            renameMessage += ": " + e;
+          }
+          throw new CorruptedFileException(tmpSnapshotFile,
+              "MD5 mismatch for snapshot-" + lastIncludedIndex + " installation.  " + renameMessage);
         } else {
           MD5FileUtil.saveMD5File(tmpSnapshotFile, digest);
         }
       }
     }
 
     if (snapshotChunkRequest.getDone()) {
-      LOG.info("Install snapshot is done, renaming tnp dir:{} to:{}",
-          tmpDir, dir.getStateMachineDir());
-      dir.getStateMachineDir().delete();
-      tmpDir.renameTo(dir.getStateMachineDir());
+      rename(tmpDir, dir.getStateMachineDir());
+    }
+  }
+
+  private static void rename(File tmpDir, File stateMachineDir) throws IOException {
+    LOG.info("Installed snapshot, renaming temporary dir {} to {}", tmpDir, stateMachineDir);
+
+    // rename stateMachineDir to tmp, if it exists.
+    final File existingDir;
+    if (stateMachineDir.exists()) {
+      File moved = null;
+      try {
+        moved = FileUtils.move(stateMachineDir, TMP + StringUtils.currentDateTime());
+      } catch(IOException e) {
+        LOG.warn("Failed to rename state machine directory " + stateMachineDir.getAbsolutePath()
+            + " to a " + TMP + " directory.  Try deleting it directly.", e);

Review comment:
       Do these warnings intentionally avoid use of placeholders?




-- 
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