Hi Andrey,
Thanks for your reply. The `ReferenceCountUtil.release` will bring
convenient in the following case
```java
ByteBuf buf;
try {
buf = allocate();
} finally {
ReferenceCountUtil.release(buf);
}
```
If there is no objections, I will start a vote to replace
`ReferenceCountUtil. safeRelease()` with
`ReferenceCountUtil.release()`.
Thanks,
Hang
Andrey Yegorov <andrey.yego...@datastax.com> 于2023年2月3日周五 08:24写道：
>
> Netty recommends to NOT use .safeRelease() unless one absolutely needs to
> swallow an exception:
> https://github.com/netty/netty/blob/48adb2609c7210ff55bb2ae42772a579052d06e7/common/src/main/java/io/netty/util/ReferenceCountUtil.java#L107-L112
>
> .release() is a convenient way to bypass null check if the object may
> happen to be null e.g. in the finally block when it is possible that
> allocation didn't happen.
> though BK's code still does the null check and then uses .release().
>
> I am not really sure why safeRelease() spread so much, maybe it was a quick
> way to deal with errors in unit tests caused by improper setup/tearDown.
>
> On an unrelated note, while looking through all that code I noticed that
> memetable uses NIO ByteBuffers.
> so naturally for the netty's ByteBuf entry has to be copied into nio buffer:
> "memTable.addEntry(ledgerId, entryId, entry.nioBuffer(), this)"
> And after getEntry() it is wrapped into netty's ByteBuf.
> I wonder if switching it to ByteBuf would improve performance / reduce java
> GC load.
>
>
> On Wed, Feb 1, 2023 at 1:28 AM Hang Chen <chenh...@apache.org> wrote:
>
> > > +1 , use new specification (ReferenceCountUtil.release) replace BP-59
> >
> > > 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.
> >
> > The `ReferenceCountUtil.release` checks whether the ByteBuf is
> > `ReferenceCounted`. If not, it will do nothing and return false.
> > ```java
> > public static boolean release(Object msg) {
> > return msg instanceof ReferenceCounted ?
> > ((ReferenceCounted)msg).release() : false;
> > }
> > ```
> > All the use cases in BookKeeper about `ReferenceCountUtil.release` are
> > not check the returned value.
> > Even Though all the ByteBuf in BookKeeper code are ReferenceCounted,
> > it still has risk of causing direct memory leak. One example is [1]
> >
> > My question is why not use `ByteBuf.release()` directly?
> >
> >
> > [1] https://github.com/apache/bookkeeper/issues/3527
> >
> > Thanks,
> > Hang
> >
> > Enrico Olivelli <eolive...@gmail.com> 于2023年2月1日周三 15:35写道：
> > >
> > > Horizon
> > >
> > > Il giorno lun 30 gen 2023 alle ore 11:14 Horizon
> > > <1060026...@qq.com.invalid> ha scritto:
> > > >
> > > > Hi, Enrico. Just confirm, only revert
> > https://github.com/apache/bookkeeper/pull/3528, it work well?
> > >
> > > yes
> > > This is the only thing we reverted in our fork
> > >
> > > see the history
> > > https://github.com/datastax/bookkeeper/commits/ds-4.14
> > >
> > > but I don't have all of the other commits in the history,
> > > so maybe other changes may be bad
> > >
> > > Unfortunately currently we (both BookKeeper community and in my
> > > company) don't have
> > > dedicated tests for BookKeeper but only for Pulsar,
> > > that makes things harder to track
> > >
> > > probably we should build some automated performance test:
> > > a set of scripts that create a BK cluster on k8s, run some
> > > benchmarks/heavy load, verify that everything worked well
> > > The workbench should be re-usable by anyone, I am not saying that we
> > > should spend the ASF resources to run
> > > this kind of tests, but that we should provide a reference load test.
> > >
> > >
> > > Enrico
> > >
> > > >
> > > > > 2023年1月30日 16:04，Enrico Olivelli <eolive...@gmail.com> 写道：
> > > > >
> > > > > I think that we must do something, current master branch is not
> > stable.
> > > > >
> > > > > My colleague Massimiliano opened this issue
> > > > > https://github.com/apache/bookkeeper/issues/3751
> > > > >
> > > > > Basically in the current master there is some problem that leads to
> > > > > Netty BytBuf corruption.
> > > > >
> > > > > The problem is solved by reverting this PR
> > > > > https://github.com/apache/bookkeeper/pull/3528 Fix memory leak when
> > > > > reading entry but the connection disconnected
> > > > >
> > > > > Enrico
> > > > >
> > > > > Il giorno lun 30 gen 2023 alle ore 08:04 steven lu
> > > > > <lushiji2...@gmail.com> ha scritto:
> > > > >>
> > > > >> There are three ways to write release in current pulsar and
> > bookkeeper
> > > > >> projects:
> > > > >> 1.ByteBuf.release: Call release directly without handling any
> > exceptions;
> > > > >> 2.ReferenceCountUtil.release: return processing status;
> > > > >> 3. ReferenceCountUtil.safeRelease: without return value, print
> > exception
> > > > >> information
> > > > >>
> > > > >> I don't think it is necessary to revert these PRs. We can discuss
> > what is
> > > > >> the correct way to release, and then modify these three ways into
> > the
> > > > >> correct way.
> > > > >>
> > > > >> Hang Chen <chenh...@apache.org> 于2023年1月30日周一 12:28写道：
> > > > >>
> > > > >>> 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
