[GitHub] [bookkeeper] hangc0276 opened a new issue, #3792: BP-61: Revert BP-59 to release ByteBuf using ReferenceCountUtil.release() instead of ReferenceCountUtil.safeRelease()

["hangc0276 (via GitHub)" <gi...@apache.org>]

hangc0276 opened a new issue, #3792:
URL: https://github.com/apache/bookkeeper/issues/3792

   **BP**
   
   > Follow the instructions at https://bookkeeper.apache.org/community/bookkeeper-proposals/ to create a proposal.
   
   This is the master ticket for tracking BP-61 :
   
   ### Motivation
   
   In BP-59, which was not discussed in the dev mail list and without a vote refactored the ByteBuf release method by ReferenceCountUtil.safeRelease() instead of ByteBuf.release().
   
   In the `ReferenceCountUtil.safeRelease()`, it catches the release exception. This change can make the ByteBuf release without any exceptions and makes the code run well, but it will make bugs hide deeper and more challenging to find out.
      ```java
      public static void safeRelease(Object msg) {
           try {
               release(msg);
           } catch (Throwable t) {
               logger.warn("Failed to release a message: {}", msg, t);
           }
       }
      ```
   For example, if there is a bug to release the ByteBuf twice, whose refcnt is 1, we can find out the bug quickly if we use ByteBuf.release(), but the bug will be hard to find out if we use `ReferenceCountUtil.safeRelease()`
   
   There are 12 PRs to refactor the ByteBuf release method, and I suggest reverting those PRs. 
   
   - https://github.com/apache/bookkeeper/pull/3673
   - https://github.com/apache/bookkeeper/pull/3674
   - https://github.com/apache/bookkeeper/pull/3687
   - https://github.com/apache/bookkeeper/pull/3688
   - https://github.com/apache/bookkeeper/pull/3689
   - https://github.com/apache/bookkeeper/pull/3691
   - https://github.com/apache/bookkeeper/pull/3693
   - https://github.com/apache/bookkeeper/pull/3694
   - https://github.com/apache/bookkeeper/pull/3695
   - https://github.com/apache/bookkeeper/pull/3698
   - https://github.com/apache/bookkeeper/pull/3700
   - https://github.com/apache/bookkeeper/pull/3703
   
   Considering some PRs have been cherry-picked into the release branch but others not, it will be hard to cherry-pick if we just push one PR to revert all the above PRs. I will push Prs one by one to replace `ReferenceCountUtil.safeRelease()` with `ReferenceCountUtil.release()`
   
   <!-- add a proposal PR link below -->
   ### Proposal PR


-- 
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-unsubscribe@bookkeeper.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [bookkeeper] Shoothzj closed issue #3792: BP-61: Revert BP-59 to release ByteBuf using ReferenceCountUtil.release() instead of ReferenceCountUtil.safeRelease()

["Shoothzj (via GitHub)" <gi...@apache.org>]

Shoothzj closed issue #3792: BP-61: Revert BP-59 to release ByteBuf using ReferenceCountUtil.release() instead of ReferenceCountUtil.safeRelease()
URL: https://github.com/apache/bookkeeper/issues/3792


-- 
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-unsubscribe@bookkeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org