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

Reply via email to