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/11/10 17:02:08 UTC
[GitHub] [incubator-ratis] szetszwo opened a new pull request #267: RATIS-1145. In NettyServerStreamRpc, the local/remote writes should only wait for the previous write.
szetszwo opened a new pull request #267:
URL: https://github.com/apache/incubator-ratis/pull/267
See https://issues.apache.org/jira/browse/RATIS-1145
----------------------------------------------------------------
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 #267: RATIS-1145. In NettyServerStreamRpc, the local/remote writes should only wait for the previous write.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #267:
URL: https://github.com/apache/incubator-ratis/pull/267#discussion_r520753354
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -410,7 +410,7 @@ static void setRetentionFileNum(RaftProperties properties, int numSnapshotFilesR
String PREFIX = RaftServerConfigKeys.PREFIX + ".data-stream";
String ASYNC_THREAD_POOL_SIZE_KEY = PREFIX + ".async.thread.pool.size";
- int ASYNC_THREAD_POOL_SIZE_DEFAULT = 4;
+ int ASYNC_THREAD_POOL_SIZE_DEFAULT = 16;
Review comment:
Any particular reason why choosing 16 threads as the number? (or why not choose a large number, like 500)?
----------------------------------------------------------------
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 #267: RATIS-1145. In NettyServerStreamRpc, the local/remote writes should only wait for the previous write.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #267:
URL: https://github.com/apache/incubator-ratis/pull/267#issuecomment-725066326
@szetszwo Thanks the patch. @amaliujia Thanks for review. I have merged 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #267: RATIS-1145. In NettyServerStreamRpc, the local/remote writes should only wait for the previous write.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #267:
URL: https://github.com/apache/incubator-ratis/pull/267#discussion_r520986364
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -429,10 +464,9 @@ private void read(ChannelHandlerContext ctx, DataStreamRequestByteBuf request) {
} else {
throw new IllegalStateException(this + ": Unexpected type " + request.getType() + ", request=" + request);
}
+ buf.release();
Review comment:
If it throws IllegalStateException, it is bug that should be fixed. Just like assertion, we won't expect the code will work correctly if an assertion fails.
----------------------------------------------------------------
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] szetszwo commented on a change in pull request #267: RATIS-1145. In NettyServerStreamRpc, the local/remote writes should only wait for the previous write.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #267:
URL: https://github.com/apache/incubator-ratis/pull/267#discussion_r520985260
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java
##########
@@ -410,7 +410,7 @@ static void setRetentionFileNum(RaftProperties properties, int numSnapshotFilesR
String PREFIX = RaftServerConfigKeys.PREFIX + ".data-stream";
String ASYNC_THREAD_POOL_SIZE_KEY = PREFIX + ".async.thread.pool.size";
- int ASYNC_THREAD_POOL_SIZE_DEFAULT = 4;
+ int ASYNC_THREAD_POOL_SIZE_DEFAULT = 16;
Review comment:
Choose 16 for a value close to the number of cores of a machine; 4 seems too small. For large value like 500, it might be good for application like Ozone but the value is too large for general applications.
----------------------------------------------------------------
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 #267: RATIS-1145. In NettyServerStreamRpc, the local/remote writes should only wait for the previous write.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267:
URL: https://github.com/apache/incubator-ratis/pull/267#discussion_r520968321
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -429,10 +464,9 @@ private void read(ChannelHandlerContext ctx, DataStreamRequestByteBuf request) {
} else {
throw new IllegalStateException(this + ": Unexpected type " + request.getType() + ", request=" + request);
}
+ buf.release();
Review comment:
If we throw IllegalStateException at previous line, we have no chance to release buf 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] runzhiwang merged pull request #267: RATIS-1145. In NettyServerStreamRpc, the local/remote writes should only wait for the previous write.
Posted by GitBox <gi...@apache.org>.
runzhiwang merged pull request #267:
URL: https://github.com/apache/incubator-ratis/pull/267
----------------------------------------------------------------
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 #267: RATIS-1145. In NettyServerStreamRpc, the local/remote writes should only wait for the previous write.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #267:
URL: https://github.com/apache/incubator-ratis/pull/267#issuecomment-725063810
@szetszwo Thanks the patch. It looks good to me overall. Just one comment added inline.
----------------------------------------------------------------
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