[GitHub] [ratis] szetszwo commented on pull request #802: RATIS-1764. [GrpcLogAppender] Automatic recover from install snapshot failure

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

szetszwo commented on PR #802:
URL: https://github.com/apache/ratis/pull/802#issuecomment-1370567614

   @SzyWilliam , the javadoc https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#createDirectories-java.nio.file.Path-java.nio.file.attribute.FileAttribute...- said that it won't throw an exception if the path already exists as a directory.  It will throw FileAlreadyExistsException when the path exists but is not a directory.  So, I think we need to delete it in this case.  Let's add `createDirectoriesDeleteExistingNonDirectory` to `FileUtils`.
   
   BTW, the `dir` parameter becomes unused after RATIS-1734.  Could you also remove it?
   
   ```java
   diff --git a/ratis-common/src/main/java/org/apache/ratis/util/FileUtils.java b/ratis-common/src/main/java/org/apache/ratis/util/FileUtils.java
   index 15ec6ea9..be736b09 100644
   --- a/ratis-common/src/main/java/org/apache/ratis/util/FileUtils.java
   +++ b/ratis-common/src/main/java/org/apache/ratis/util/FileUtils.java
   @@ -71,6 +71,21 @@ public interface FileUtils {
            () -> "Files.createDirectories " + dir);
      }
    
   +  static void createDirectoriesDeleteExistingNonDirectory(File dir) throws IOException {
   +    createDirectoriesDeleteExistingNonDirectory(dir.toPath());
   +  }
   +
   +  static void createDirectoriesDeleteExistingNonDirectory(Path dir) throws IOException {
   +    try {
   +      createDirectories(dir);
   +    } catch (FileAlreadyExistsException e) {
   +      LOG.warn("Failed to create directory " + dir
   +          + " since it already exists as a non-directory.  Trying to delete it ...", e);
   +      delete(dir);
   +      createDirectories(dir);
   +    }
   +  }
   +
      static void move(File src, File dst) throws IOException {
        move(src.toPath(), dst.toPath());
      }
   diff --git a/ratis-server/src/main/java/org/apache/ratis/server/impl/ServerState.java b/ratis-server/src/main/java/org/apache/ratis/server/impl/ServerState.java
   index 826c4cff..e0832c9b 100644
   --- a/ratis-server/src/main/java/org/apache/ratis/server/impl/ServerState.java
   +++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/ServerState.java
   @@ -441,7 +441,7 @@ class ServerState {
        // TODO: verify that we need to install the snapshot
        StateMachine sm = server.getStateMachine();
        sm.pause(); // pause the SM to prepare for install snapshot
   -    snapshotManager.installSnapshot(request, sm, getStorage().getStorageDir());
   +    snapshotManager.installSnapshot(request, sm);
        updateInstalledSnapshotIndex(TermIndex.valueOf(request.getSnapshotChunk().getTermIndex()));
      }
    
   diff --git a/ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java b/ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java
   index dbde8694..4ee94bf6 100644
   --- a/ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java
   +++ b/ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java
   @@ -77,8 +77,7 @@ public class SnapshotManager {
            new File(dir.get().getRoot(), c.getFilename()).toPath()).toString();
      }
    
   -  public void installSnapshot(InstallSnapshotRequestProto request, StateMachine stateMachine, RaftStorageDirectory dir)
   -      throws IOException {
   +  public void installSnapshot(InstallSnapshotRequestProto request, StateMachine stateMachine) throws IOException {
        final InstallSnapshotRequestProto.SnapshotChunkProto snapshotChunkRequest = request.getSnapshotChunk();
        final long lastIncludedIndex = snapshotChunkRequest.getTermIndex().getIndex();
    
   @@ -102,7 +101,7 @@ public class SnapshotManager {
          }
    
          final File tmpSnapshotFile = new File(tmpDir, getRelativePath.apply(chunk));
   -      FileUtils.createDirectories(tmpSnapshotFile);
   +      FileUtils.createDirectoriesDeleteExistingNonDirectory(tmpSnapshotFile);
    
          FileOutputStream out = null;
          try {
   ```


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