You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ratis.apache.org by GitBox <gi...@apache.org> on 2020/08/05 06:39:11 UTC
[GitHub] [incubator-ratis] amaliujia opened a new pull request #168: [RATIS-1023] During installing snapshot, when snapshot file is corrup…
amaliujia opened a new pull request #168:
URL: https://github.com/apache/incubator-ratis/pull/168
…ted, rename the temp snapshot file to .corrupt.
## What changes were proposed in this pull request?
It is possible that snapshot file contains corrupted data, in this case within `installSnapshot`, we can rename the temp snapshot file with `.corrupt` before throwing an exception.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1023
Please replace this section with the link to the Apache JIRA)
## How was this patch tested?
unit test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] bshashikant commented on pull request #168: RATIS-1023.During installing snapshot, when snapshot file is corrup…
Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #168:
URL: https://github.com/apache/incubator-ratis/pull/168#issuecomment-672784212
Thanks @amaliujia for the contribution and @runzhiwang for the review.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] bshashikant merged pull request #168: RATIS-1023.During installing snapshot, when snapshot file is corrup…
Posted by GitBox <gi...@apache.org>.
bshashikant merged pull request #168:
URL: https://github.com/apache/incubator-ratis/pull/168
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] bshashikant commented on pull request #168: RATIS-1023.During installing snapshot, when snapshot file is corrup…
Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #168:
URL: https://github.com/apache/incubator-ratis/pull/168#issuecomment-669309968
Thanks Rui Wang for starting working on this. Can you please add some details as to why renaming temp snapshot file to .corrupt is required?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on pull request #168: [RATIS-1023] During installing snapshot, when snapshot file is corrup…
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #168:
URL: https://github.com/apache/incubator-ratis/pull/168#issuecomment-669036900
@amaliujia Please change your title to **RATIS-1023. During installing snapshot, when snapshot file is corrup**
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] amaliujia commented on pull request #168: [RATIS-1023] During installing snapshot, when snapshot file is corrup…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #168:
URL: https://github.com/apache/incubator-ratis/pull/168#issuecomment-669012107
@bshashikant could you take a look?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] amaliujia commented on a change in pull request #168: RATIS-1023.During installing snapshot, when snapshot file is corrup…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #168:
URL: https://github.com/apache/incubator-ratis/pull/168#discussion_r465859007
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java
##########
@@ -131,4 +133,12 @@ public void installSnapshot(StateMachine stateMachine,
tmpDir.renameTo(dir.getStateMachineDir());
}
}
+
+ // Rename a file by appending .corrupt to file name. This function does not guarantee
+ // that the rename operation is successful.
+ @VisibleForTesting
+ static void renameFileToCorrupt(File tmpSnapshotFile) {
Review comment:
Thanks for your reminder.
I didn't find such function implemented in common util, but indeed I found something useful so I pushed to adopt those. I also moved the rename-to-corrupt to FileUtils.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #168: [RATIS-1023] During installing snapshot, when snapshot file is corrup…
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #168:
URL: https://github.com/apache/incubator-ratis/pull/168#discussion_r465537587
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java
##########
@@ -131,4 +133,12 @@ public void installSnapshot(StateMachine stateMachine,
tmpDir.renameTo(dir.getStateMachineDir());
}
}
+
+ // Rename a file by appending .corrupt to file name. This function does not guarantee
+ // that the rename operation is successful.
+ @VisibleForTesting
+ static void renameFileToCorrupt(File tmpSnapshotFile) {
Review comment:
why static ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] amaliujia commented on pull request #168: [RATIS-1023] During installing snapshot, when snapshot file is corrup…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #168:
URL: https://github.com/apache/incubator-ratis/pull/168#issuecomment-669011760
This is my first time to submit a patch to Ratis. Any suggestion is much appreciated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] amaliujia commented on pull request #168: RATIS-1023.During installing snapshot, when snapshot file is corrup…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #168:
URL: https://github.com/apache/incubator-ratis/pull/168#issuecomment-669317208
Thanks @bshashikant! I updated the JIRA and we can discuss there.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #168: RATIS-1023.During installing snapshot, when snapshot file is corrup…
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #168:
URL: https://github.com/apache/incubator-ratis/pull/168#discussion_r465549811
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java
##########
@@ -131,4 +133,12 @@ public void installSnapshot(StateMachine stateMachine,
tmpDir.renameTo(dir.getStateMachineDir());
}
}
+
+ // Rename a file by appending .corrupt to file name. This function does not guarantee
+ // that the rename operation is successful.
+ @VisibleForTesting
+ static void renameFileToCorrupt(File tmpSnapshotFile) {
Review comment:
I think it should be a common util, and should not placed here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] amaliujia commented on a change in pull request #168: RATIS-1023.During installing snapshot, when snapshot file is corrup…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #168:
URL: https://github.com/apache/incubator-ratis/pull/168#discussion_r465540479
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java
##########
@@ -131,4 +133,12 @@ public void installSnapshot(StateMachine stateMachine,
tmpDir.renameTo(dir.getStateMachineDir());
}
}
+
+ // Rename a file by appending .corrupt to file name. This function does not guarantee
+ // that the rename operation is successful.
+ @VisibleForTesting
+ static void renameFileToCorrupt(File tmpSnapshotFile) {
Review comment:
Because this function does not access any class member. Any suggestion or style guide that says better to not use static?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #168: RATIS-1023.During installing snapshot, when snapshot file is corrup…
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #168:
URL: https://github.com/apache/incubator-ratis/pull/168#discussion_r465549811
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java
##########
@@ -131,4 +133,12 @@ public void installSnapshot(StateMachine stateMachine,
tmpDir.renameTo(dir.getStateMachineDir());
}
}
+
+ // Rename a file by appending .corrupt to file name. This function does not guarantee
+ // that the rename operation is successful.
+ @VisibleForTesting
+ static void renameFileToCorrupt(File tmpSnapshotFile) {
Review comment:
I think rename should be a common util, and should not placed here. Maybe current ratis has implemented this function (I'm not sure), you can call it directly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org