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 > >>> >
