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 2022/02/07 06:32:13 UTC
[GitHub] [ratis] guohao-rosicky opened a new pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
guohao-rosicky opened a new pull request #596:
URL: https://github.com/apache/ratis/pull/596
## What changes were proposed in this pull request?
When DataStreamManagement#read an exception occurs, remove DataStream
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1519
--
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] guohao-rosicky commented on a change in pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on a change in pull request #596:
URL: https://github.com/apache/ratis/pull/596#discussion_r801209865
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -439,7 +448,7 @@ private void readImpl(DataStreamRequestByteBuf request, ChannelHandlerContext ct
}, requestExecutor)).whenComplete((v, exception) -> {
try {
if (exception != null) {
- streams.remove(key);
+ removeDataStream(request);
Review comment:
Yes, I modify 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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] guohao-rosicky edited a comment on pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
guohao-rosicky edited a comment on pull request #596:
URL: https://github.com/apache/ratis/pull/596#issuecomment-1034550231
@szetszwo
in `org.apache.ratis.datastream.DataStreamTestUtils#assertHeader` `stateMachine.getSingleDataStream` in null, I found that it will be added at `stateMachine.stream`, I wonder why not get 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.
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 pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #596:
URL: https://github.com/apache/ratis/pull/596#issuecomment-1033376346
@guohao-rosicky , from the debug message you added, it is a Mock for RaftServer. It also showed "DataStream null". that We probably need to update the test.
```
2022-02-09 13:43:51,230 [Time-limited test] INFO datastream.DataStreamTestUtils
(DataStreamTestUtils.java:writeAndCloseAndAssertReplies(315)) -
test header: RaftClientRequest:client-992CD7CB0548->s0@group-9E34F31FA0DB, cid=0, seq=0,
DataStream, null,
server: Mock for RaftServer, hashCode: 1696083569, bytesWritten: 7452308, stepDownLeader: false
```
--
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] guohao-rosicky commented on pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on pull request #596:
URL: https://github.com/apache/ratis/pull/596#issuecomment-1031127624
@szetszwo Please take a look.
--
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] guohao-rosicky commented on pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on pull request #596:
URL: https://github.com/apache/ratis/pull/596#issuecomment-1041025620
@szetszwo I apply the patch, I still have an NPE, I'll keep tracking the issues
--
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 edited a comment on pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
szetszwo edited a comment on pull request #596:
URL: https://github.com/apache/ratis/pull/596#issuecomment-1032169663
@guohao-rosicky , there are some NPEs in https://github.com/apache/ratis/runs/5103000516?check_suite_focus=true . Please take a look. We may have to update the tests?
--
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 pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #596:
URL: https://github.com/apache/ratis/pull/596#issuecomment-1041103968
> ... I apply the patch, I still have an NPE, ...
@guohao-rosicky , Do you mean https://issues.apache.org/jira/secure/attachment/13039878/596_debug.patch ? It just prints out some dubug messages. Please check if the new behavior is correct. If yes, we can update the test in order to avoid the NPE.
--
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 change in pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #596:
URL: https://github.com/apache/ratis/pull/596#discussion_r800848928
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -240,6 +240,18 @@ StreamInfo remove(ClientInvocationId key) {
return f;
}
+ private void removeDataStream(DataStreamRequestByteBuf request) {
+ try {
+ final ClientInvocationId invocationId = ClientInvocationId.valueOf(request.getClientId(), request.getStreamId());
+ final StreamInfo s = streams.remove(invocationId);
+ if (s != null) {
+ s.getDivision().getDataStreamMap().remove(invocationId);
Review comment:
Just found that we could avoid s.getDivision() to throw IOException; see https://issues.apache.org/jira/secure/attachment/13039740/596_review.patch
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -439,7 +448,7 @@ private void readImpl(DataStreamRequestByteBuf request, ChannelHandlerContext ct
}, requestExecutor)).whenComplete((v, exception) -> {
try {
if (exception != null) {
- streams.remove(key);
+ removeDataStream(request);
Review comment:
We should pass `info` in this case since it could be already removed from `streams` at https://github.com/apache/ratis/blob/d41d0930c49c4cfcd8cd709519329e22fbbe61b4/ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java#L401
--
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] guohao-rosicky edited a comment on pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
guohao-rosicky edited a comment on pull request #596:
URL: https://github.com/apache/ratis/pull/596#issuecomment-1034550231
@szetszwo
in `org.apache.ratis.datastream.DataStreamTestUtils#assertHeader` `stateMachine.getSingleDataStream` in null, I found that it will be added at `stateMachine.stream`, I wonder why not get it.
Can you help me find 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.
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 pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #596:
URL: https://github.com/apache/ratis/pull/596#issuecomment-1032169663
@guohao-rosicky , there are some NPEs in https://github.com/apache/ratis/runs/5103000516?check_suite_focus=true . Please take a look.
--
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] guohao-rosicky commented on pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on pull request #596:
URL: https://github.com/apache/ratis/pull/596#issuecomment-1031327544
@szetszwo modified. I removed `streams.remove ()` from `readImpl`, Please take a look
--
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] guohao-rosicky closed pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
guohao-rosicky closed pull request #596:
URL: https://github.com/apache/ratis/pull/596
--
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 change in pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #596:
URL: https://github.com/apache/ratis/pull/596#discussion_r800408732
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -379,6 +390,7 @@ void read(DataStreamRequestByteBuf request, ChannelHandlerContext ctx,
try {
readImpl(request, ctx, buf, getStreams);
} catch (Throwable t) {
+ removeDataStream(buf);
buf.release();
Review comment:
Let's have buf.release() first.
```
@@ -380,6 +392,7 @@ public class DataStreamManagement {
readImpl(request, ctx, buf, getStreams);
} catch (Throwable t) {
buf.release();
+ removeDataStream(request);
throw t;
}
}
```
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -240,6 +240,17 @@ StreamInfo remove(ClientInvocationId key) {
return f;
}
+ private void removeDataStream(ByteBuf buf) {
+ try {
+ RaftClientRequest request = ClientProtoUtils.toRaftClientRequest(
+ RaftClientRequestProto.parseFrom(buf.nioBuffer()));
Review comment:
This works only if request.getType() == Type.STREAM_HEADER. The code should be:
```
private void removeDataStream(DataStreamRequestByteBuf request) {
try {
final ClientInvocationId invocationId = ClientInvocationId.valueOf(request.getClientId(), request.getStreamId());
final StreamInfo s = streams.remove(invocationId);
if (s != null) {
s.getDivision().getDataStreamMap().remove(invocationId);
}
} catch (IOException ignored) {
LOG.debug(this + ": Failed to removeDataStream for " + request, ignored);
}
}
```
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -240,6 +240,17 @@ StreamInfo remove(ClientInvocationId key) {
return f;
}
+ private void removeDataStream(ByteBuf buf) {
+ try {
+ RaftClientRequest request = ClientProtoUtils.toRaftClientRequest(
+ RaftClientRequestProto.parseFrom(buf.nioBuffer()));
+ ClientInvocationId invocationId = ClientInvocationId.valueOf(request);
+ server.getDivision(request.getRaftGroupId()).getDataStreamMap().remove(invocationId);
+ } catch (Throwable e) {
+ throw new CompletionException(e);
Review comment:
Just print an error message as shown above.
--
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 pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #596:
URL: https://github.com/apache/ratis/pull/596#issuecomment-1034885140
@guohao-rosicky , It seems the server behavior was changed. The primary did not forward the request to the next server as shown below that s1 returns null; see https://issues.apache.org/jira/secure/attachment/13039878/596_debug.patch
```
2022-02-10 20:37:26,654 [Time-limited test] INFO datastream.DataStreamTestUtils (DataStreamTestUtils.java:writeAndCloseAndAssertReplies(314))
- start Stream0
2022-02-10 20:37:26,683 [nioEventLoopGroup-3-1] INFO statemachine.StateMachine (DataStreamTestUtils.java:stream(151))
- XXX MultiDataStreamStateMachine:s0:group-56A12F14DED8 put 0@client-E43970695D43, SingleDataStream:
writeRequest=RaftClientRequest:client-E43970695D43->s0@group-56A12F14DED8, cid=0, seq=0, DataStream, Message:<EMPTY>, logEntry=null
2022-02-10 20:37:26,736 [Time-limited test] INFO datastream.DataStreamTestUtils (DataStreamTestUtils.java:assertHeader(331))
- XXX s0: dataSize=7478599, stepDownLeader=false, header=RaftClientRequest:client-E43970695D43->s0@group-56A12F14DED8, cid=0, seq=0, DataStream, null
2022-02-10 20:37:26,736 [Time-limited test] INFO statemachine.StateMachine (DataStreamTestUtils.java:getSingleDataStream(181))
- XXX MultiDataStreamStateMachine:s0:group-56A12F14DED8: get 0@client-E43970695D43 return SingleDataStream:
writeRequest=RaftClientRequest:client-E43970695D43->s0@group-56A12F14DED8, cid=0, seq=0, DataStream, Message:<EMPTY>, logEntry=null
2022-02-10 20:37:26,737 [Time-limited test] INFO datastream.DataStreamTestUtils (DataStreamTestUtils.java:assertHeader(331))
- XXX s1: dataSize=7478599, stepDownLeader=false, header=RaftClientRequest:client-E43970695D43->s0@group-56A12F14DED8, cid=0, seq=0, DataStream, null
2022-02-10 20:37:26,737 [Time-limited test] INFO statemachine.StateMachine (DataStreamTestUtils.java:getSingleDataStream(181))
- XXX MultiDataStreamStateMachine:s1:group-56A12F14DED8: get 0@client-E43970695D43 return null
```
Please check if the new behavior is correct. If yes, we can update the test.
--
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] guohao-rosicky commented on pull request #596: RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream
Posted by GitBox <gi...@apache.org>.
guohao-rosicky commented on pull request #596:
URL: https://github.com/apache/ratis/pull/596#issuecomment-1034550231
> Please take a look, 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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org