szetszwo commented on a change in pull request #272:
URL: https://github.com/apache/incubator-ratis/pull/272#discussion_r521897365
##########
File path:
ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -326,8 +327,11 @@ private StreamInfo newStreamInfo(ByteBuf buf) {
}
static long writeTo(ByteBuf buf, DataStream stream) {
- final WritableByteChannel channel = stream.getWritableByteChannel();
long byteWritten = 0;
+ if (stream == null) {
Review comment:
MultiDataStreamStateMachine is static so that we can use it directly
without refactoring. If it is desirable, we may do the refactoring later.
> ... I found that without this NULL check, this code will hit a NPE and the
program will hang there. ...
The NULL check is hiding but not fixing the problem. For fixing the
problem, we should return the NPE back to the client. As mentioned previously,
we need to work on the exception handling once the basic feature has been
completed.
##########
File path:
ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -326,8 +327,11 @@ private StreamInfo newStreamInfo(ByteBuf buf) {
}
static long writeTo(ByteBuf buf, DataStream stream) {
- final WritableByteChannel channel = stream.getWritableByteChannel();
long byteWritten = 0;
+ if (stream == null) {
Review comment:
If the test coverage MiniRaftCluster becomes a super set of
TestDataStreamNetty, we should remove TestDataStreamNetty. This is an
engineering process. We start with TestDataStreamNetty and keep adding
features. Our goal is to make Streaming working with a real cluster.
Let's keep both tests for a moment and review if we should remove
TestDataStreamNetty later on.
----------------------------------------------------------------
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:
[email protected]
