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/10/21 03:20:01 UTC
[GitHub] [incubator-ratis] szetszwo opened a new pull request #228: RATIS-1098. DataStreamReply should include byteWritten and isSuccess.
szetszwo opened a new pull request #228:
URL: https://github.com/apache/incubator-ratis/pull/228
See https://issues.apache.org/jira/browse/RATIS-1098
----------------------------------------------------------------
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 #228: RATIS-1098. DataStreamReply should include byteWritten and isSuccess.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #228:
URL: https://github.com/apache/incubator-ratis/pull/228#discussion_r509346878
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -212,7 +211,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws IOExceptio
JavaUtils.allOf(remoteWrites).thenCombine(localWrite, (v, bytesWritten) -> {
buf.release();
- sendReply(request, ctx);
+ sendReply(request, bytesWritten, ctx);
Review comment:
That is a good idea. When bytesWritten is different, it should retry to send the remaining data. Let's work on it separately since it needs some works.
----------------------------------------------------------------
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 #228: RATIS-1098. DataStreamReply should include byteWritten and isSuccess.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #228:
URL: https://github.com/apache/incubator-ratis/pull/228#discussion_r509633336
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -212,7 +211,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws IOExceptio
JavaUtils.allOf(remoteWrites).thenCombine(localWrite, (v, bytesWritten) -> {
buf.release();
- sendReply(request, ctx);
+ sendReply(request, bytesWritten, ctx);
Review comment:
SG thanks!
----------------------------------------------------------------
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 merged pull request #228: RATIS-1098. DataStreamReply should include byteWritten and isSuccess.
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #228:
URL: https://github.com/apache/incubator-ratis/pull/228
----------------------------------------------------------------
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 #228: RATIS-1098. DataStreamReply should include byteWritten and isSuccess.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #228:
URL: https://github.com/apache/incubator-ratis/pull/228#issuecomment-713839178
LGTM
----------------------------------------------------------------
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 #228: RATIS-1098. DataStreamReply should include byteWritten and isSuccess.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #228:
URL: https://github.com/apache/incubator-ratis/pull/228#discussion_r509012392
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -212,7 +211,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws IOExceptio
JavaUtils.allOf(remoteWrites).thenCombine(localWrite, (v, bytesWritten) -> {
buf.release();
- sendReply(request, ctx);
+ sendReply(request, bytesWritten, ctx);
Review comment:
It might be useful to check whether `bytesWritten` of local write is equal to `bytesWritten` from replies from remote writes to peers as a correct check.
----------------------------------------------------------------
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 #228: RATIS-1098. DataStreamReply should include byteWritten and isSuccess.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #228:
URL: https://github.com/apache/incubator-ratis/pull/228#discussion_r509012392
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -212,7 +211,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws IOExceptio
JavaUtils.allOf(remoteWrites).thenCombine(localWrite, (v, bytesWritten) -> {
buf.release();
- sendReply(request, ctx);
+ sendReply(request, bytesWritten, ctx);
Review comment:
It might be useful to check whether `bytesWritten` of local write is equal to `bytesWritten` to peers as a correct check.
----------------------------------------------------------------
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 edited a comment on pull request #228: RATIS-1098. DataStreamReply should include byteWritten and isSuccess.
Posted by GitBox <gi...@apache.org>.
szetszwo edited a comment on pull request #228:
URL: https://github.com/apache/incubator-ratis/pull/228#issuecomment-713958383
Thanks @runzhiwang and @amaliujia for reviewing 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] szetszwo commented on pull request #228: RATIS-1098. DataStreamReply should include byteWritten and isSuccess.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #228:
URL: https://github.com/apache/incubator-ratis/pull/228#issuecomment-713958383
Thanks @runzhiwang and @szetszwo for reviewing 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