+1 , use new specification (ReferenceCountUtil.release) replace BP-59
On 2023/01/30 09:24:03 Shiji Lu wrote: > Good suggestions, > We currently have several ways to fix this problem, revert all previous PRs, > or directly replace them with new standard methods, I am ok, we can vote. > > I think one specification can be used at present, all of which are released > in the way of ReferenceCountUtil.release, replacing these three ways > > On 2023/01/30 08:04:46 Enrico Olivelli wrote: > > 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 > > > > > > >
