Copilot commented on code in PR #16027:
URL: https://github.com/apache/dubbo/pull/16027#discussion_r2707966248
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyBackedChannelBuffer.java:
##########
@@ -239,13 +249,18 @@ public void readBytes(ChannelBuffer dst, int length) {
@Override
public void readBytes(ChannelBuffer dst, int dstIndex, int length) {
- // careful
if (readableBytes() < length) {
throw new IndexOutOfBoundsException();
}
- byte[] data = new byte[length];
- buffer.readBytes(data, 0, length);
- dst.setBytes(dstIndex, data, 0, length);
+ // use Netty ByteBuf
+ if (dst instanceof NettyBackedChannelBuffer) {
+ NettyBackedChannelBuffer nettyDst = (NettyBackedChannelBuffer) dst;
+ buffer.readBytes(nettyDst.buffer, dstIndex, length);
+ } else {
+ byte[] data = new byte[length];
+ buffer.readBytes(data, 0, length);
+ dst.setBytes(dstIndex, data, 0, length);
+ }
Review Comment:
The optimized path for NettyBackedChannelBuffer in readBytes lacks
comprehensive test coverage. While the existing testBufferTransfer test
exercises setBytes with two NettyBackedChannelBuffer instances, there's no test
specifically covering the readBytes method with both NettyBackedChannelBuffer
(optimized path) and other ChannelBuffer implementations (fallback path) to
verify correctness of both code branches.
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyBackedChannelBuffer.java:
##########
@@ -362,10 +377,15 @@ public void writeBytes(ChannelBuffer src, int length) {
@Override
public void writeBytes(ChannelBuffer src, int srcIndex, int length) {
- // careful
- byte[] data = new byte[length];
- src.getBytes(srcIndex, data, 0, length);
- writeBytes(data, 0, length);
+ // use Netty ByteBuf
+ if (src instanceof NettyBackedChannelBuffer) {
+ NettyBackedChannelBuffer nettySrc = (NettyBackedChannelBuffer) src;
+ buffer.writeBytes(nettySrc.buffer, srcIndex, length);
+ } else {
+ byte[] data = new byte[length];
+ src.getBytes(srcIndex, data, 0, length);
+ writeBytes(data, 0, length);
+ }
Review Comment:
The optimized path for NettyBackedChannelBuffer in writeBytes lacks
comprehensive test coverage. While the existing testBufferTransfer test
exercises setBytes with two NettyBackedChannelBuffer instances, there's no test
specifically covering the writeBytes method with both NettyBackedChannelBuffer
(optimized path) and other ChannelBuffer implementations (fallback path) to
verify correctness of both code branches.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
