poorbarcode commented on code in PR #4289: URL: https://github.com/apache/bookkeeper/pull/4289#discussion_r1567862280
########## bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java: ########## @@ -1180,6 +1180,7 @@ private void writeAndFlush(final Channel channel, if (channel == null) { LOG.warn("Operation {} failed: channel == null", StringUtils.requestToString(request)); errorOut(key); + ReferenceCountUtil.release(request); Review Comment: To trace the context easier, I move the second comment here. > I think it's the wrong thing to do generally in this method. I'll share a new PR where I explain an alternative approach that handles both the V3 protocol and the UnsafeByteOperations issue and the issue that was found. > I don't think that the current changes in this PR are ok. I created https://github.com/apache/bookkeeper/pull/4293 to show what I think that should be done. I added a section named `Explanation` in the Motivation, it explains that the current change is correct. Thanks for mentioning me and greater suggestions ❤️ ########## bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java: ########## @@ -1180,6 +1180,7 @@ private void writeAndFlush(final Channel channel, if (channel == null) { LOG.warn("Operation {} failed: channel == null", StringUtils.requestToString(request)); errorOut(key); + ReferenceCountUtil.release(request); Review Comment: To trace the context easier, I move the second comment here. > I think it's the wrong thing to do generally in this method. I'll share a new PR where I explain an alternative approach that handles both the V3 protocol and the UnsafeByteOperations issue and the issue that was found. > I don't think that the current changes in this PR are ok. I created https://github.com/apache/bookkeeper/pull/4293 to show what I think that should be done. I added a section named `Explanation` in the Motivation, it explains that the current change is correct. Thanks for mentioning me and great suggestions ❤️ -- 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: commits-unsubscr...@bookkeeper.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org