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

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

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



##########
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:
       You mean "{}"?  Unfortunately, if we use "{}", then the stack trace of the exception won't be printed.
   
   It is easy to have bug like below:
   ```
   LOG.warn{"a: {}, b: {}", a, b, e);
   ```
   The exception e won't be printed at all.
   




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