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 2021/01/29 19:57:11 UTC
[GitHub] [incubator-ratis] alpapad opened a new pull request #411: Fixes problems with large snapshots
alpapad opened a new pull request #411:
URL: https://github.com/apache/incubator-ratis/pull/411
## What changes were proposed in this pull request?
Hi,
I have been testing ratis for a solutions which uses rather largish snapshots for the state machine. Current implementation is not able to transfer files larger than 8Mb (I think), throwing all kinds of exceptions. I guess this is something not really used by current users of ratis.
Here are the places I modified/found to be problematic (code is more precise than words):
FileChunkReader.readFileChunk uses ByteString.readFrom(in, chunkLength); which will fully load the input stream and not just a junk of size chunkLength
Grpc.proto service installSnapshot(stream ratis.common.InstallSnapshotRequestProto) should return a stream
Modifications to SnapshotManager so chunks for the same snapshot will be placed in the same directory
Not sure if I need to create a jira issue (never used apache f. jira) I can repro and create jiras with stacktraces etc.
----------------------------------------------------------------
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] alpapad commented on a change in pull request #411: Fixes problems with large snapshots
Posted by GitBox <gi...@apache.org>.
alpapad commented on a change in pull request #411:
URL: https://github.com/apache/incubator-ratis/pull/411#discussion_r567062290
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/FileChunkReader.java
##########
@@ -80,6 +81,32 @@ public FileChunkProto readFileChunk(int chunkMaxSize) throws IOException {
return proto;
}
+ /**
Review comment:
Code copied from ByteString class
----------------------------------------------------------------
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 #411: Fixes problems with large snapshots
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #411:
URL: https://github.com/apache/incubator-ratis/pull/411#issuecomment-770140979
For such optimizations, usually I think it worths a have a benchmark to identify the bottleneck and/or provide a baseline to convince people that such optimization is useful, and how much it is useful.
----------------------------------------------------------------
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] alpapad commented on a change in pull request #411: Fixes problems with large snapshots
Posted by GitBox <gi...@apache.org>.
alpapad commented on a change in pull request #411:
URL: https://github.com/apache/incubator-ratis/pull/411#discussion_r567063376
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java
##########
@@ -130,10 +130,9 @@ public void installSnapshot(StateMachine stateMachine,
}
if (snapshotChunkRequest.getDone()) {
- LOG.info("Install snapshot is done, renaming tnp dir:{} to:{}",
- tmpDir, dir.getStateMachineDir());
- dir.getStateMachineDir().delete();
- tmpDir.renameTo(dir.getStateMachineDir());
+ LOG.info("Install snapshot is done, moving files from dir:{} to:{}", tmpDir, dir.getStateMachineDir());
Review comment:
Handles the case where multiple files are copied
----------------------------------------------------------------
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] alpapad commented on a change in pull request #411: Fixes problems with large snapshots
Posted by GitBox <gi...@apache.org>.
alpapad commented on a change in pull request #411:
URL: https://github.com/apache/incubator-ratis/pull/411#discussion_r567062092
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/FileChunkReader.java
##########
@@ -65,7 +66,7 @@ public FileChunkReader(FileInfo info, RaftStorageDirectory directory) throws IOE
public FileChunkProto readFileChunk(int chunkMaxSize) throws IOException {
final long remaining = info.getFileSize() - offset;
final int chunkLength = remaining < chunkMaxSize ? (int) remaining : chunkMaxSize;
- final ByteString data = ByteString.readFrom(in, chunkLength);
Review comment:
ByteString.readFrom(in, chunkLength) will read the whole file not just chunkLength
----------------------------------------------------------------
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 #411: Fixes problems with large snapshots
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #411:
URL: https://github.com/apache/incubator-ratis/pull/411#issuecomment-770067456
@alpapad would you mind creating a JIRA for this PR?
----------------------------------------------------------------
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 #411: Fixes problems with large snapshots
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #411:
URL: https://github.com/apache/incubator-ratis/pull/411#issuecomment-770068902
I guess this will worth an email to user@ratis to ask the problem you are facing.
I don't think Ratis cannot handle larger than 8MB files transferred well. There is an example in Ratis repo named FileStore. You can use that example to save files by an application based on Ratis. I believe I saw people used GBs files on FileStore.
----------------------------------------------------------------
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] alpapad commented on a change in pull request #411: Fixes problems with large snapshots
Posted by GitBox <gi...@apache.org>.
alpapad commented on a change in pull request #411:
URL: https://github.com/apache/incubator-ratis/pull/411#discussion_r567062731
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java
##########
@@ -61,22 +60,23 @@ public void installSnapshot(StateMachine stateMachine,
final long lastIncludedIndex = snapshotChunkRequest.getTermIndex().getIndex();
final RaftStorageDirectory dir = storage.getStorageDir();
- // create a unique temporary directory
- final File tmpDir = new File(dir.getTmpDir(), UUID.randomUUID().toString());
Review comment:
when receiving a snapshot which does not fit in a single chunk, then multiple directories are creates, each having a chunk of the snapshot
----------------------------------------------------------------
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 edited a comment on pull request #411: Fixes problems with large snapshots
Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #411:
URL: https://github.com/apache/incubator-ratis/pull/411#issuecomment-770140979
For such optimizations, usually I think it worths a benchmark to identify the bottleneck and/or provide a baseline to convince people that such optimization is useful, and how much it is useful.
----------------------------------------------------------------
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] alpapad commented on pull request #411: Fixes problems with large snapshots
Posted by GitBox <gi...@apache.org>.
alpapad commented on pull request #411:
URL: https://github.com/apache/incubator-ratis/pull/411#issuecomment-770117213
@amaliujia The problem exists when a statemachine snapshot needs to be transfered (for example because of a node failure). The snapshot should be chunked to a specific chunk-size but instead the whole file is read and the server does not expect it to be more than X Mb (I think 8, but I will need to check).
I think the FileStore statemachine does not store much and file contents are not even stored in the ratis wal.
I will create a jira for this.
----------------------------------------------------------------
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 edited a comment on pull request #411: Fixes problems with large snapshots
Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #411:
URL: https://github.com/apache/incubator-ratis/pull/411#issuecomment-770068902
I guess this will worth an email to user@ratis to ask the problem you are facing.
I don't think Ratis cannot handle larger than 8MB files transferred well. There is an example in Ratis repo named FileStore. You can use that example to save files by an application based on Ratis. I believe I saw people used GBs files on FileStore. Ratis is also used by Apache Ozone which is supposed to stress Ratis a lot in its use cases.
----------------------------------------------------------------
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] alpapad commented on a change in pull request #411: Fixes problems with large snapshots
Posted by GitBox <gi...@apache.org>.
alpapad commented on a change in pull request #411:
URL: https://github.com/apache/incubator-ratis/pull/411#discussion_r567063641
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java
##########
@@ -61,22 +60,23 @@ public void installSnapshot(StateMachine stateMachine,
final long lastIncludedIndex = snapshotChunkRequest.getTermIndex().getIndex();
final RaftStorageDirectory dir = storage.getStorageDir();
- // create a unique temporary directory
- final File tmpDir = new File(dir.getTmpDir(), UUID.randomUUID().toString());
+ // create a unique temporary directory based on the request id
+ final File tmpDir = new File(dir.getTmpDir(), snapshotChunkRequest.getRequestId());
FileUtils.createDirectories(tmpDir);
tmpDir.deleteOnExit();
- LOG.info("Installing snapshot:{}, to tmp dir:{}", request, tmpDir);
Review comment:
This will log the whole chunk.. Lots of bytes..
----------------------------------------------------------------
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] alpapad commented on a change in pull request #411: Fixes problems with large snapshots
Posted by GitBox <gi...@apache.org>.
alpapad commented on a change in pull request #411:
URL: https://github.com/apache/incubator-ratis/pull/411#discussion_r567062731
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/storage/SnapshotManager.java
##########
@@ -61,22 +60,23 @@ public void installSnapshot(StateMachine stateMachine,
final long lastIncludedIndex = snapshotChunkRequest.getTermIndex().getIndex();
final RaftStorageDirectory dir = storage.getStorageDir();
- // create a unique temporary directory
- final File tmpDir = new File(dir.getTmpDir(), UUID.randomUUID().toString());
Review comment:
when receiving a snapshot which does not fit in a single chunk, then multiple directories are created, each having a chunk of the snapshot
----------------------------------------------------------------
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