Re: [DISCUSS] BP-61: Revert BP-59 to release ByteBuf using ByteBuf.release() instead of ReferenceCountUtil.safeRelease()

[Andrey Yegorov <an...@datastax.com>]

Hi,

I think it makes sense to replace ReferenceCountUtil.safeRelease
with ReferenceCountUtil.release.
FWIW ReferenceCountUtil.release's return value just says if the byte buf's
ref count reached zero or not, it is not an error handling mechanism.

safeRelease logs at warn level. Have you seen these messages in your prod
clusters so we can say this an actual problem and not a generic concern
(for prioritization)?

On Sun, Jan 29, 2023 at 8:28 PM Hang Chen <ch...@apache.org> wrote:

> Hi guys,
>    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. Do you have any ideas?
>
> 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
>
>
> Thanks,
> Hang
>


-- 
Andrey Yegorov