You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "SzyWilliam (via GitHub)" <gi...@apache.org> on 2023/04/25 02:09:46 UTC
[GitHub] [ratis] SzyWilliam opened a new pull request, #877: RATIS-1837. Restrict reading maxChunkSize bytes each installSnapshot RPC
SzyWilliam opened a new pull request, #877:
URL: https://github.com/apache/ratis/pull/877
See https://issues.apache.org/jira/browse/RATIS-1837
The observations refer to issue comments in https://github.com/apache/ratis/pull/876.
The cause is that `ByteString::readFrom` will drain out the inputStream and reads all data of a stream into memory, as stated in the doc https://javadoc.io/doc/com.google.protobuf/protobuf-java/2.5.0/com/google/protobuf/ByteString.html
> Completely reads the given stream's bytes into a ByteString, blocking if necessary until all bytes are read through to the end of the stream.
When a snapshot file is of Gigabytes size, this `ByteString::readFrom` takes 6s to complete and by the time it returns, the follower already starves and starts a new election.
--
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] adoroszlai merged pull request #877: RATIS-1837. Restrict reading maxChunkSize bytes each installSnapshot RPC
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #877:
URL: https://github.com/apache/ratis/pull/877
--
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] adoroszlai commented on a diff in pull request #877: RATIS-1837. Restrict reading maxChunkSize bytes each installSnapshot RPC
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #877:
URL: https://github.com/apache/ratis/pull/877#discussion_r1177406229
##########
ratis-server/src/main/java/org/apache/ratis/server/storage/FileChunkReader.java:
##########
@@ -47,11 +51,14 @@ public class FileChunkReader implements Closeable {
*
* @param info the information of the file.
* @param relativePath the relative path of the file.
+ * @param chunkMaxSize maximum chunk size for each {@link #readFileChunk()} call
* @throws IOException if it failed to open the file.
*/
- public FileChunkReader(FileInfo info, Path relativePath) throws IOException {
+ public FileChunkReader(FileInfo info, Path relativePath, int chunkMaxSize) throws IOException {
this.info = info;
this.relativePath = relativePath;
+ this.chunkMaxSize = chunkMaxSize;
Review Comment:
Would it make sense to limit this if file is smaller, i.e. `min(chunkMaxSize, info.getFileSize())`?
--
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] adoroszlai commented on pull request #877: RATIS-1837. Restrict reading maxChunkSize bytes each installSnapshot RPC
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #877:
URL: https://github.com/apache/ratis/pull/877#issuecomment-1524906482
Thanks @SzyWilliam for the patch, @szetszwo 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.
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 #877: RATIS-1837. Restrict reading maxChunkSize bytes each installSnapshot RPC
Posted by "SzyWilliam (via GitHub)" <gi...@apache.org>.
SzyWilliam commented on code in PR #877:
URL: https://github.com/apache/ratis/pull/877#discussion_r1177985363
##########
ratis-server/src/main/java/org/apache/ratis/server/storage/FileChunkReader.java:
##########
@@ -47,11 +51,14 @@ public class FileChunkReader implements Closeable {
*
* @param info the information of the file.
* @param relativePath the relative path of the file.
+ * @param chunkMaxSize maximum chunk size for each {@link #readFileChunk()} call
* @throws IOException if it failed to open the file.
*/
- public FileChunkReader(FileInfo info, Path relativePath) throws IOException {
+ public FileChunkReader(FileInfo info, Path relativePath, int chunkMaxSize) throws IOException {
this.info = info;
this.relativePath = relativePath;
+ this.chunkMaxSize = chunkMaxSize;
Review Comment:
You are right! We can save some unnecessary memory allocations.
--
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 #877: RATIS-1837. Restrict reading maxChunkSize bytes each installSnapshot RPC
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #877:
URL: https://github.com/apache/ratis/pull/877#discussion_r1178072913
##########
ratis-server/src/main/java/org/apache/ratis/server/storage/FileChunkReader.java:
##########
@@ -65,14 +72,16 @@ public FileChunkReader(FileInfo info, Path relativePath) throws IOException {
/**
* Read the next chunk.
*
- * @param chunkMaxSize maximum chunk size
* @return the chunk read from the file.
* @throws IOException if it failed to read the file.
*/
- public FileChunkProto readFileChunk(int chunkMaxSize) throws IOException {
+ public FileChunkProto readFileChunk() throws IOException {
final long remaining = info.getFileSize() - offset;
final int chunkLength = remaining < chunkMaxSize ? (int) remaining : chunkMaxSize;
- final ByteString data = ByteString.readFrom(in, chunkLength);
+ final int bytesRead = in.read(chunkBuffer, 0, chunkLength);
Review Comment:
Let's use `IOUtils.readFully`. It will read in a loop.
##########
ratis-server/src/main/java/org/apache/ratis/server/storage/FileChunkReader.java:
##########
@@ -47,11 +51,14 @@ public class FileChunkReader implements Closeable {
*
* @param info the information of the file.
* @param relativePath the relative path of the file.
+ * @param chunkMaxSize maximum chunk size for each {@link #readFileChunk()} call
* @throws IOException if it failed to open the file.
*/
- public FileChunkReader(FileInfo info, Path relativePath) throws IOException {
+ public FileChunkReader(FileInfo info, Path relativePath, int chunkMaxSize) throws IOException {
this.info = info;
this.relativePath = relativePath;
+ this.chunkMaxSize = Math.min(chunkMaxSize, (int) info.getFileSize());
+ this.chunkBuffer = new byte[chunkMaxSize];
Review Comment:
Since `ByteString` is supposed to be immutable, `FileChunkProto.Builder` won't copy the `data` in its `build()` method. If we only have a single buffer, we can only have a single outstanding `FileChunkProto` (and a single outstanding `InstallSnapshotRequestProto`). Otherwise, if we read a second `FileChunkProto`, the `data` in first `FileChunkProto` will be changed silently. (That's why it is named `UnsafeByteOperations`.)
--
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 #877: RATIS-1837. Restrict reading maxChunkSize bytes each installSnapshot RPC
Posted by "SzyWilliam (via GitHub)" <gi...@apache.org>.
SzyWilliam commented on PR #877:
URL: https://github.com/apache/ratis/pull/877#issuecomment-1524496771
@szetszwo Thanks for the suggestions. Simplicity and safety indeed outweigh throughput since snapshot is a rare operation. Made corresponding changes on code. Please take a look again.
--
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