RATIS-1078. Refactor DataStream classes. #210
Conversation
| @@ -29,25 +29,22 @@ | |||
|
|
|||
| @Override | |||
| protected void decode(ChannelHandlerContext channelHandlerContext, | |||
| ByteBuf byteBuf, List<Object> list) throws Exception { | |||
| ByteBuf byteBuf, List<Object> list) { | |||
|
|
|||
| if(byteBuf.readableBytes() >= 24){ | |||
There was a problem hiding this comment.
Out of curiosity: why this magical number 24?
There was a problem hiding this comment.
Ah I see. It seems that there are at least 3 long at the beginning of the buffer to serve as metadata. That's why here checks >= 24 bytes.
It will be useful to have javadoc on encode/decode functions to document the encoding format.
There was a problem hiding this comment.
This may be a good suggestion but is not related to this issue.
| @@ -1,4 +1,4 @@ | |||
| /** | |||
| /* | |||
There was a problem hiding this comment.
not sure about this, most project use /**
There was a problem hiding this comment.
See https://www.apache.org/licenses/LICENSE-2.0
the text should be enclosed in the appropriate comment syntax for the file format.
It requires comment syntax. If we use /**, the license will be shown in the generated javadoc. It probably is not something that we want.
There was a problem hiding this comment.
+1 for /*. With /** IntelliJ IDEA also warns about "Dangling Javadoc comment".
| } | ||
| } else{ |
There was a problem hiding this comment.
if byteBuf.readableBytes() >= 24 is false, do we need to print err log ?
There was a problem hiding this comment.
This may be a good suggestion but is not related to this issue. This is a refactoring but not try to change anything to the code.
| DataStreamRequestServer req = new DataStreamRequestServer(streamId, | ||
| dataOffset, | ||
| bf); | ||
| final DataStreamRequestByteBuf req = new DataStreamRequestByteBuf(streamId, dataOffset, bf); | ||
| byteBuf.readerIndex(byteBuf.readerIndex() + (int)dataLength); | ||
| byteBuf.markReaderIndex(); | ||
| list.add(req); | ||
| } else { | ||
| byteBuf.resetReaderIndex(); |
There was a problem hiding this comment.
if byteBuf.readableBytes() >= dataLength is false, do we need to print err log ?
There was a problem hiding this comment.
This may be a good suggestion but is not related to this issue. This is a refactoring but not try to change anything to the code.
| } | ||
| } else{ |
There was a problem hiding this comment.
if byteBuf.readableBytes() >= 24 is false, do we need to print err log ?
There was a problem hiding this comment.
This may be a good suggestion but is not related to this issue. This is a refactoring but not try to change anything to the code.
ratis-netty/src/main/java/org/apache/ratis/netty/client/DataStreamReplyDecoder.java
Show resolved
Hide resolved
|
@szetszwo Thanks for the patch. It looks good to me overall. Just a few comments added inline. |
|
@runzhiwang , thanks for reviewing this. This issue is for refactoring but not try to change anything to the code as stated in the description of https://issues.apache.org/jira/browse/RATIS-1078 . |
|
LGTM as this is a just a refactoring. We can fix useful suggestions in the another Jira. |
|
@szetszwo Thanks for the patch, I have merged it. @amaliujia Thanks for review. |
|
@runzhiwang , @amaliujia thanks a lot for reviewing this. |
Co-authored-by: Tsz Wo Sze <tszwosze@Tszs-MacBook-Pro.local>
See https://issues.apache.org/jira/browse/RATIS-1078