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/22 04:28:01 UTC
[GitHub] [incubator-ratis] amaliujia opened a new pull request #230: RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful
amaliujia opened a new pull request #230:
URL: https://github.com/apache/incubator-ratis/pull/230
## What changes were proposed in this pull request?
Add a correctness check that compares the `byteswritten` to peers with current `byteswritten` locally. If remote write is not successful or `byteswritten` is not equal, should return a reply with "not success".
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1104
## How was this patch tested?
UT
----------------------------------------------------------------
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 #230: RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #230:
URL: https://github.com/apache/incubator-ratis/pull/230#discussion_r509914757
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -199,25 +207,45 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws IOExceptio
// do not need to forward header request
final List<DataStreamOutput> outs = proxies.getDataStreamOutput();
peersStreamOutput.put(request.getStreamId(), outs);
- for(DataStreamOutput out : outs) {
+ for (DataStreamOutput out : outs) {
remoteWrites.add(out.getHeaderFuture());
}
} else {
// body
- for(DataStreamOutput out : peersStreamOutput.get(request.getStreamId())) {
+ for (DataStreamOutput out : peersStreamOutput.get(request.getStreamId())) {
remoteWrites.add(out.writeAsync(request.slice().nioBuffer()));
}
}
JavaUtils.allOf(remoteWrites).thenCombine(localWrite, (v, bytesWritten) -> {
buf.release();
- sendReply(request, bytesWritten, ctx);
Review comment:
You can
sendReply(request, bytesWritten, ctx);
You can move the following code into sendReply, and rename the origin sendReply to sendReplySuccess.
```
try {
if (!checkSuccessRemoteWrite(remoteWrites, bytesWritten)) {
sendReplyNotSuccess(request, ctx);
} else {
sendReply(request, bytesWritten, ctx);
}
} catch (ExecutionException | InterruptedException e) {
IOUtils.asIOException(e);
}
```
----------------------------------------------------------------
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 #230: RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #230:
URL: https://github.com/apache/incubator-ratis/pull/230#issuecomment-715042367
@amaliujia , thanks a lot for working on this.
@runzhiwang , let's keep only the first line in the commit log. Otherwise, the commit log becomes too long.
```
commit ad3cd5ae6c731d2f797890614a4f7bbcaa3ae818 (origin/master, origin/HEAD)
Author: Rui Wang <am...@users.noreply.github.com>
Date: Thu Oct 22 00:41:20 2020 -0700
RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful (#230)
* RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful
* fixup! reorder imports
* fixup! address comments
* fixup! address comments
* fixup! fix style
```
----------------------------------------------------------------
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 #230: RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #230:
URL: https://github.com/apache/incubator-ratis/pull/230#discussion_r509891703
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -211,6 +219,17 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws IOExceptio
JavaUtils.allOf(remoteWrites).thenCombine(localWrite, (v, bytesWritten) -> {
buf.release();
+ for (CompletableFuture<DataStreamReply> replyFuture : remoteWrites) {
Review comment:
Please define another method to do 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] runzhiwang commented on pull request #230: RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #230:
URL: https://github.com/apache/incubator-ratis/pull/230#issuecomment-715046209
@szetszwo Thanks for explanation. I will keep only the first line in the commit log in the future.
----------------------------------------------------------------
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 #230: RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #230:
URL: https://github.com/apache/incubator-ratis/pull/230#discussion_r509914757
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -199,25 +207,45 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws IOExceptio
// do not need to forward header request
final List<DataStreamOutput> outs = proxies.getDataStreamOutput();
peersStreamOutput.put(request.getStreamId(), outs);
- for(DataStreamOutput out : outs) {
+ for (DataStreamOutput out : outs) {
remoteWrites.add(out.getHeaderFuture());
}
} else {
// body
- for(DataStreamOutput out : peersStreamOutput.get(request.getStreamId())) {
+ for (DataStreamOutput out : peersStreamOutput.get(request.getStreamId())) {
remoteWrites.add(out.writeAsync(request.slice().nioBuffer()));
}
}
JavaUtils.allOf(remoteWrites).thenCombine(localWrite, (v, bytesWritten) -> {
buf.release();
- sendReply(request, bytesWritten, ctx);
Review comment:
You can move the following code into sendReply, and rename the origin sendReply to sendReplySuccess.
```
try {
if (!checkSuccessRemoteWrite(remoteWrites, bytesWritten)) {
sendReplyNotSuccess(request, ctx);
} else {
sendReply(request, bytesWritten, ctx);
}
} catch (ExecutionException | InterruptedException e) {
IOUtils.asIOException(e);
}
```
----------------------------------------------------------------
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 #230: RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #230:
URL: https://github.com/apache/incubator-ratis/pull/230#discussion_r509927843
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -199,25 +207,45 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws IOExceptio
// do not need to forward header request
final List<DataStreamOutput> outs = proxies.getDataStreamOutput();
peersStreamOutput.put(request.getStreamId(), outs);
- for(DataStreamOutput out : outs) {
+ for (DataStreamOutput out : outs) {
remoteWrites.add(out.getHeaderFuture());
}
} else {
// body
- for(DataStreamOutput out : peersStreamOutput.get(request.getStreamId())) {
+ for (DataStreamOutput out : peersStreamOutput.get(request.getStreamId())) {
remoteWrites.add(out.writeAsync(request.slice().nioBuffer()));
}
}
JavaUtils.allOf(remoteWrites).thenCombine(localWrite, (v, bytesWritten) -> {
buf.release();
- sendReply(request, bytesWritten, ctx);
Review comment:
I see. Updated. 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] amaliujia commented on a change in pull request #230: RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #230:
URL: https://github.com/apache/incubator-ratis/pull/230#discussion_r509927843
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -199,25 +207,45 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws IOExceptio
// do not need to forward header request
final List<DataStreamOutput> outs = proxies.getDataStreamOutput();
peersStreamOutput.put(request.getStreamId(), outs);
- for(DataStreamOutput out : outs) {
+ for (DataStreamOutput out : outs) {
remoteWrites.add(out.getHeaderFuture());
}
} else {
// body
- for(DataStreamOutput out : peersStreamOutput.get(request.getStreamId())) {
+ for (DataStreamOutput out : peersStreamOutput.get(request.getStreamId())) {
remoteWrites.add(out.writeAsync(request.slice().nioBuffer()));
}
}
JavaUtils.allOf(remoteWrites).thenCombine(localWrite, (v, bytesWritten) -> {
buf.release();
- sendReply(request, bytesWritten, ctx);
Review comment:
I see. Updated.
----------------------------------------------------------------
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 #230: RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful
Posted by GitBox <gi...@apache.org>.
runzhiwang merged pull request #230:
URL: https://github.com/apache/incubator-ratis/pull/230
----------------------------------------------------------------
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 #230: RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #230:
URL: https://github.com/apache/incubator-ratis/pull/230#issuecomment-714298844
@amaliujia Thanks the patch. 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] amaliujia commented on pull request #230: RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #230:
URL: https://github.com/apache/incubator-ratis/pull/230#issuecomment-714216524
R @szetszwo @runzhiwang
Can you take a look please?
----------------------------------------------------------------
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 #230: RATIS-1104.Should check "bytewritten" from remote writes to peers to determine whether current stream write is successful
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #230:
URL: https://github.com/apache/incubator-ratis/pull/230#discussion_r509914757
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -199,25 +207,45 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws IOExceptio
// do not need to forward header request
final List<DataStreamOutput> outs = proxies.getDataStreamOutput();
peersStreamOutput.put(request.getStreamId(), outs);
- for(DataStreamOutput out : outs) {
+ for (DataStreamOutput out : outs) {
remoteWrites.add(out.getHeaderFuture());
}
} else {
// body
- for(DataStreamOutput out : peersStreamOutput.get(request.getStreamId())) {
+ for (DataStreamOutput out : peersStreamOutput.get(request.getStreamId())) {
remoteWrites.add(out.writeAsync(request.slice().nioBuffer()));
}
}
JavaUtils.allOf(remoteWrites).thenCombine(localWrite, (v, bytesWritten) -> {
buf.release();
- sendReply(request, bytesWritten, ctx);
Review comment:
You can
sendReply(request, bytesWritten, ctx);
You can move the following code into sendReply, and rename the origin sendReply to sendReplySuccess.
```
try {
if (!checkSuccessRemoteWrite(remoteWrites, bytesWritten)) {
sendReplyNotSuccess(request, ctx);
} else {
sendReply(request, bytesWritten, ctx);
}
} catch (ExecutionException | InterruptedException e) {
IOUtils.asIOException(e);
}
```
----------------------------------------------------------------
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