You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2022/06/05 15:53:27 UTC
[GitHub] [ratis] SzyWilliam opened a new pull request, #655: RATIS-1587. Fix snapshot multi-chunk bug & support snapshot hierarchy
SzyWilliam opened a new pull request, #655:
URL: https://github.com/apache/ratis/pull/655
## What changes were proposed in this pull request?
* Fix snapshot multiple-chunk bug. Currently, when leader install a snapshot(multiple chunks) to a newly joined follower, leader will send multiple InstallSnapshot RPCs. However, each RPC will create a tmp dir with Random UUID, place the chunk in this tmp dir, and only renames the last tmp dir to sm-dir. In this PR, I propose to create tmp dir using request.uuid(), which remains unchanged among multiple RPCs.
* Fix Grpc Stream errors. Currently In grpc.proto, InstallSnapshot is declaimed as client-end streaming rpc, but it is actual bi-directional streaming rpc. In this PR, I addded stream to InstallSnapshot proto so that it becomes bi-directional.
* Support snapshot file hierarchy. Currently all files of a snapshot will be placed in statemachine dir and file hierarchy is flattened. In this PR, I name each file using its original filename (which contains hierarchy information).
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1587
## How was this patch tested?
* Unit Tests. It passed all unit tests in my machine (MacOS 12.13)
* Manual Test. I packaged this version of ratis and tested its snapshot functionality in our project Apache IoTDB, and everything is working as expected
--
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
[GitHub] [ratis] SzyWilliam commented on a diff in pull request #655: RATIS-1587. Fix snapshot multi-chunk bug & support snapshot hierarchy
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on code in PR #655:
URL: https://github.com/apache/ratis/pull/655#discussion_r890711657
##########
ratis-proto/src/main/proto/Grpc.proto:
##########
@@ -44,7 +44,7 @@ service RaftServerProtocolService {
returns(stream ratis.common.AppendEntriesReplyProto) {}
rpc installSnapshot(stream ratis.common.InstallSnapshotRequestProto)
- returns(ratis.common.InstallSnapshotReplyProto) {}
+ returns(stream ratis.common.InstallSnapshotReplyProto) {}
Review Comment:
I think it is a bug when using GRPC as communication protocol and involves multiple snapshot chunks. The handler of InstallSnapshot is implemented bidirectional-streaming, but the proto is declaimed client-streaming. The mismatch will cause the leader received a HTTP RST CANCEL for the last installSnapshot request.
--
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
[GitHub] [ratis] SzyWilliam commented on pull request #655: RATIS-1587. Fix snapshot multi-chunk bug & support snapshot hierarchy
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on PR #655:
URL: https://github.com/apache/ratis/pull/655#issuecomment-1148302984
@szetszwo I added unit test which covers the scenario of Leader InstallSnapshot to Followers with snapshot containing multiple files in nested folder.
--
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
[GitHub] [ratis] szetszwo merged pull request #655: RATIS-1587. Fix snapshot multi-chunk bug & support snapshot hierarchy
Posted by GitBox <gi...@apache.org>.
szetszwo merged PR #655:
URL: https://github.com/apache/ratis/pull/655
--
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
[GitHub] [ratis] szetszwo commented on a diff in pull request #655: RATIS-1587. Fix snapshot multi-chunk bug & support snapshot hierarchy
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #655:
URL: https://github.com/apache/ratis/pull/655#discussion_r891641082
##########
ratis-proto/src/main/proto/Grpc.proto:
##########
@@ -44,7 +44,7 @@ service RaftServerProtocolService {
returns(stream ratis.common.AppendEntriesReplyProto) {}
rpc installSnapshot(stream ratis.common.InstallSnapshotRequestProto)
- returns(ratis.common.InstallSnapshotReplyProto) {}
+ returns(stream ratis.common.InstallSnapshotReplyProto) {}
Review Comment:
I agree that this is a bug. If it is an incompatible change, we still have to document it even for a bug since it will break existing, working applications. Currently, applications with single InstallSnapshotRequestProto are working.
Indeed, it may not be incompatible. Could you test it if an old server (without `stream`) can talk to a new server (with `stream`) ?
--
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
[GitHub] [ratis] szetszwo commented on a diff in pull request #655: RATIS-1587. Fix snapshot multi-chunk bug & support snapshot hierarchy
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #655:
URL: https://github.com/apache/ratis/pull/655#discussion_r890353887
##########
ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java:
##########
@@ -83,9 +83,10 @@ public void installSnapshot(StateMachine stateMachine,
}
String fileName = chunk.getFilename(); // this is relative to the root dir
- // TODO: assumes flat layout inside SM dir
- File tmpSnapshotFile = new File(tmpDir,
- new File(dir.getRoot(), fileName).getName());
+ String fileNameToStateMachineDir = fileName.substring(
+ (dir.STATE_MACHINE_DIR_NAME.length()));
Review Comment:
We need handle the path separator. How about using Path to relativize it?
```
@@ -74,6 +74,7 @@ public class SnapshotManager {
// TODO: Make sure that subsequent requests for the same installSnapshot are coming in order,
// and are not lost when whole request cycle is done. Check requestId and requestIndex here
+ final Path stateMachineDir = dir.getStateMachineDir().toPath();
for (FileChunkProto chunk : snapshotChunkRequest.getFileChunksList()) {
SnapshotInfo pi = stateMachine.getLatestSnapshot();
if (pi != null && pi.getTermIndex().getIndex() >= lastIncludedIndex) {
@@ -83,9 +84,9 @@ public class SnapshotManager {
}
String fileName = chunk.getFilename(); // this is relative to the root dir
- // TODO: assumes flat layout inside SM dir
- File tmpSnapshotFile = new File(tmpDir,
- new File(dir.getRoot(), fileName).getName());
+ final Path relative = stateMachineDir.relativize(new File(dir.getRoot(), fileName).toPath());
+ final File tmpSnapshotFile = new File(tmpDir, relative.toString());
+ FileUtils.createDirectories(tmpSnapshotFile);
```
##########
ratis-proto/src/main/proto/Grpc.proto:
##########
@@ -44,7 +44,7 @@ service RaftServerProtocolService {
returns(stream ratis.common.AppendEntriesReplyProto) {}
rpc installSnapshot(stream ratis.common.InstallSnapshotRequestProto)
- returns(ratis.common.InstallSnapshotReplyProto) {}
+ returns(stream ratis.common.InstallSnapshotReplyProto) {}
Review Comment:
Is this an incompatible change? If yes, we should document it.
--
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
[GitHub] [ratis] szetszwo commented on pull request #655: RATIS-1587. Fix snapshot multi-chunk bug & support snapshot hierarchy
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #655:
URL: https://github.com/apache/ratis/pull/655#issuecomment-1149453283
@SzyWilliam , thanks a lot for testing the compatibility!
--
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
[GitHub] [ratis] SzyWilliam commented on pull request #655: RATIS-1587. Fix snapshot multi-chunk bug & support snapshot hierarchy
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on PR #655:
URL: https://github.com/apache/ratis/pull/655#issuecomment-1149397010
@szetszwo Add License header. Also, I did the test and I think the `stream` change is compatible. That's how I did the test:
I compile the Arithmetic example in `ratis-examples` both with&w/o `stream`. Then I started Arithmetic Server w/o stream as leader, apply trxs, take a snaphot and purge logs. After that I started Arithmetic Server with stream and join the group. I discovered that the leader successfully install the snapshot to the new follower, which means old server w/o stream can install single chunk snapshot to new server with stream.
--
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
[GitHub] [ratis] szetszwo commented on a diff in pull request #655: RATIS-1587. Fix snapshot multi-chunk bug & support snapshot hierarchy
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #655:
URL: https://github.com/apache/ratis/pull/655#discussion_r891641842
##########
ratis-server/src/test/java/org/apache/ratis/InstallSnapshotFromLeaderTests.java:
##########
@@ -0,0 +1,154 @@
+package org.apache.ratis;
Review Comment:
Need license header.
--
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
[GitHub] [ratis] SzyWilliam commented on a diff in pull request #655: RATIS-1587. Fix snapshot multi-chunk bug & support snapshot hierarchy
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on code in PR #655:
URL: https://github.com/apache/ratis/pull/655#discussion_r890709845
##########
ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java:
##########
@@ -83,9 +83,10 @@ public void installSnapshot(StateMachine stateMachine,
}
String fileName = chunk.getFilename(); // this is relative to the root dir
- // TODO: assumes flat layout inside SM dir
- File tmpSnapshotFile = new File(tmpDir,
- new File(dir.getRoot(), fileName).getName());
+ String fileNameToStateMachineDir = fileName.substring(
+ (dir.STATE_MACHINE_DIR_NAME.length()));
Review Comment:
It would be great, I'll take this way
--
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