[GitHub] [ratis] SzyWilliam commented on a diff in pull request #774: RATIS-1734. Allow StateMachine to decide snapshot install path

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

SzyWilliam commented on code in PR #774:
URL: https://github.com/apache/ratis/pull/774#discussion_r1013701752


##########
ratis-server/src/main/java/org/apache/ratis/server/storage/FileChunkReader.java:
##########
@@ -50,11 +50,11 @@ public class FileChunkReader implements Closeable {
    * @param directory the directory where the file is stored.
    * @throws IOException if it failed to open the file.
    */
-  public FileChunkReader(FileInfo info, RaftStorageDirectory directory) throws IOException {
+  public FileChunkReader(FileInfo info, File directory) throws IOException {
     this.info = info;
     this.relativePath = Optional.of(info.getPath())
         .filter(Path::isAbsolute)
-        .map(p -> directory.getRoot().toPath().relativize(p))
+        .map(p -> directory.toPath().relativize(p))

Review Comment:
   Thanks for reminding me of incompatibility issues and providing the patch!
   I have a small question. Is it possible that we also let StateMachine provide `RootDir`? In this way, we can still have a unified handling logic in receiver end `SnapshotManager`, while we don't have to add `sm` prefix to the sender side. However, this may introduce a new restriction on StateMachine: the `snapshotDir` must be a children of `rootDir`. What do you think?



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