RATIS-1519. When DataStreamManagement#read an exception occurs, remove DataStream by guohao-rosicky · Pull Request #596 · apache/ratis

guohao-rosicky · 2022-02-07T06:31:59Z

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

guohao-rosicky · 2022-02-07T06:45:19Z

@szetszwo Please take a look.

szetszwo

@guohao-rosicky , thanks for working on this. Some comments inlined.

szetszwo · 2022-02-07T08:26:01Z

+      removeDataStream(buf);
      buf.release();


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; } }

szetszwo · 2022-02-07T08:30:06Z

+      RaftClientRequest request = ClientProtoUtils.toRaftClientRequest(
+          RaftClientRequestProto.parseFrom(buf.nioBuffer()));


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); } }

szetszwo · 2022-02-07T08:30:51Z

+      ClientInvocationId invocationId = ClientInvocationId.valueOf(request);
+      server.getDivision(request.getRaftGroupId()).getDataStreamMap().remove(invocationId);
+    } catch (Throwable e) {
+      throw new CompletionException(e);


Just print an error message as shown above.

guohao-rosicky · 2022-02-07T10:46:51Z

@szetszwo modified. I removed streams.remove () from readImpl, Please take a look

szetszwo

@guohao-rosicky , good catch. Some comments inlined; see also https://issues.apache.org/jira/secure/attachment/13039740/596_review.patch

szetszwo · 2022-02-07T16:42:59Z

+      final ClientInvocationId invocationId = ClientInvocationId.valueOf(request.getClientId(), request.getStreamId());
+      final StreamInfo s = streams.remove(invocationId);
+      if (s != null) {
+        s.getDivision().getDataStreamMap().remove(invocationId);


Just found that we could avoid s.getDivision() to throw IOException; see https://issues.apache.org/jira/secure/attachment/13039740/596_review.patch

szetszwo · 2022-02-07T16:47:28Z

      try {
        if (exception != null) {
-          streams.remove(key);
+          removeDataStream(request);


We should pass info in this case since it could be already removed from streams at

ratis/ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java

Line 401 in d41d093

info = Optional.ofNullable(streams.remove(key)).orElseThrow(

Yes, I modify it

szetszwo · 2022-02-08T03:15:14Z

@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?

szetszwo · 2022-02-09T05:49:05Z

@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

szetszwo

Please take a look, thanks.

guohao-rosicky · 2022-02-10T06:40:22Z

@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.

szetszwo · 2022-02-10T12:50:44Z

@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.

…e DataStream

guohao-rosicky · 2022-02-16T02:21:59Z

@szetszwo I apply the patch, I still have an NPE, I'll keep tracking the issues

szetszwo · 2022-02-16T04:49:28Z

... 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.

szetszwo · 2023-11-02T16:55:59Z

@guohao-rosicky , there are some conflicts. Could you fix them?

szetszwo

+1 the change looks good.

szetszwo · 2024-04-08T18:11:53Z

@guohao-rosicky , thanks for working not this! On top of the bug fix, the change from RaftServer to Division is great!

…e DataStream (apache#596)

szetszwo requested changes Feb 7, 2022

View reviewed changes

szetszwo reviewed Feb 7, 2022

View reviewed changes

guohao-rosicky force-pushed the guohao-RATIS-1519-dev branch from ef63a6f to d2c486c Compare February 8, 2022 02:05

guohao-rosicky requested a review from szetszwo February 8, 2022 02:09

szetszwo reviewed Feb 9, 2022

View reviewed changes

guohao-rosicky closed this Feb 10, 2022

guohao-rosicky reopened this Feb 10, 2022

RATIS-1519. When DataStreamManagement#read an exception occurs, remov…

f5b0905

…e DataStream

guohao-rosicky force-pushed the guohao-RATIS-1519-dev branch from 1ca5047 to f5b0905 Compare February 15, 2022 03:51

trigger new CI

fbc9b3a

Merge branch 'master' into guohao-RATIS-1519-dev

892ea63

szetszwo approved these changes Apr 8, 2024

View reviewed changes

szetszwo merged commit bc6221b into apache:master Apr 8, 2024

symious pushed a commit to symious/ratis that referenced this pull request Apr 12, 2024

RATIS-1519. When DataStreamManagement#read an exception occurs, remov…

f93a909

…e DataStream (apache#596)

szetszwo pushed a commit to szetszwo/ratis that referenced this pull request Jun 16, 2024

RATIS-1519. When DataStreamManagement#read an exception occurs, remov…

caa35dd

…e DataStream (apache#596)

		RaftClientRequest request = ClientProtoUtils.toRaftClientRequest(
		RaftClientRequestProto.parseFrom(buf.nioBuffer()));

Conversation

guohao-rosicky commented Feb 7, 2022

What changes were proposed in this pull request?

What is the link to the Apache JIRA

Uh oh!

guohao-rosicky commented Feb 7, 2022

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

szetszwo Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

szetszwo Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

szetszwo Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

guohao-rosicky commented Feb 7, 2022

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

szetszwo Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

szetszwo Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

guohao-rosicky Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

szetszwo commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szetszwo commented Feb 9, 2022

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

guohao-rosicky commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szetszwo commented Feb 10, 2022

Uh oh!

guohao-rosicky commented Feb 16, 2022

Uh oh!

szetszwo commented Feb 16, 2022

Uh oh!

szetszwo commented Nov 2, 2023

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

szetszwo commented Apr 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

szetszwo commented Feb 8, 2022 •

edited

Loading

guohao-rosicky commented Feb 10, 2022 •

edited

Loading