You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@bookkeeper.apache.org by Yunze Xu <yz...@streamnative.io.INVALID> on 2021/05/04 09:22:26 UTC

Cutting BookKeeper 4.13.1 or 4.14.0 release?

Hello,
About 10 days ago I found a heap memory copy problem in Apache Pulsar, see [1].
It’s a problem of BK side because when `LedgerHandle#asyncAddEntry` accepts a `CompositeByteBuf` or a wrapper, it will finally call `ByteBuf#nioBuffer()`, which would make a heap copy from direct memory.
[2] fixed this problem and has been merged for a week.

Since it has a significant impact on Pulsar, Pulsar side needs a new BK release with [2] merged to fix it. Is there any plan to cut a 4.13.1 release or 4.14.0 release so that we can upgrade the dependency in Pulsar?

Thanks,
Yunze

[1] https://github.com/apache/pulsar/pull/10330 <https://github.com/apache/pulsar/pull/10330>

[2] https://github.com/apache/bookkeeper/pull/2701 <https://github.com/apache/bookkeeper/pull/2701>

Re: Cutting BookKeeper 4.13.1 or 4.14.0 release?

Posted by Enrico Olivelli <eo...@gmail.com>.

Il giorno mar 4 mag 2021 alle ore 11:22 Yunze Xu
<yz...@streamnative.io.invalid> ha scritto:
>
> Hello,
> About 10 days ago I found a heap memory copy problem in Apache Pulsar, see [1].
> It’s a problem of BK side because when `LedgerHandle#asyncAddEntry` accepts a `CompositeByteBuf` or a wrapper, it will finally call `ByteBuf#nioBuffer()`, which would make a heap copy from direct memory.
> [2] fixed this problem and has been merged for a week.

+1 for a release and then ship Pulsar 2.8.0 with new the BookKeeper

I believe we should just find a volunteer as RM at this point

Enrico

>
> Since it has a significant impact on Pulsar, Pulsar side needs a new BK release with [2] merged to fix it. Is there any plan to cut a 4.13.1 release or 4.14.0 release so that we can upgrade the dependency in Pulsar?
>
> Thanks,
> Yunze
>
> [1] https://github.com/apache/pulsar/pull/10330 <https://github.com/apache/pulsar/pull/10330>
>
> [2] https://github.com/apache/bookkeeper/pull/2701 <https://github.com/apache/bookkeeper/pull/2701>
>

Re: Cutting BookKeeper 4.13.1 or 4.14.0 release?

Posted by Enrico Olivelli <eo...@gmail.com>.

Amit,

Il Sab 15 Mag 2021, 15:05 Amit Chavan <ac...@gmail.com> ha scritto:

> +1 for release of bookkeeper 4.14.
> All tests pass for herddb. PR here - Upgrade to BK 4.14
> <https://github.com/diennea/herddb/pull/750>
>

Thank you for casting your vote and testing HerdDB

But the right VOTE thread is
https://lists.apache.org/x/thread.html/r48224f8f1f4be0de244aaeff339213feac18ce6278382f12b929e432@%3Cdev.bookkeeper.apache.org%3E

Please post your message as answer to that message

Enrico




> On Mon, May 10, 2021 at 2:17 AM Andrey Yegorov <
> andrey.yegorov@datastax.com>
> wrote:
>
> > I noticed that PRs are in so I built the RC0 while I had time.
> > Please vote on the corresponding thread.
> >
> > On Fri, May 7, 2021 at 3:15 PM Andrey Yegorov <
> andrey.yegorov@datastax.com
> > >
> > wrote:
> >
> > > Matteo,
> > >
> > > https://github.com/apache/bookkeeper/pull/2649 also a good candidate
> for
> > > this release (fits the compaction improvements theme), also has changes
> > > requested
> > >
> > > On Fri, May 7, 2021 at 3:01 PM Andrey Yegorov <
> > andrey.yegorov@datastax.com>
> > > wrote:
> > >
> > >> ok, I'll hold on on the release until these two are ready to merge
> > >>
> > >> Both PRs have changes requested by Enrico.
> > >> I'll postpone my attempt to build the release until Monday.
> > >>
> > >> If there are other PRs that *have* to be in 4.14 please move them back
> > to
> > >> the 4.14 milestone and update this thread.
> > >>
> > >>
> > >> On Fri, May 7, 2021 at 1:06 PM Matteo Merli <mm...@apache.org>
> wrote:
> > >>
> > >>> Andrey, there are several PRs that would be good to get into 4.14. We
> > >>> shouldn't just push everything out to 4.15.
> > >>>
> > >>> Just a couple of examples:
> > >>>  * https://github.com/apache/bookkeeper/pull/2710
> > >>>  * https://github.com/apache/bookkeeper/pull/2698
> > >>>
> > >>>
> > >>>
> > >>> --
> > >>> Matteo Merli
> > >>> <mm...@apache.org>
> > >>>
> > >>> On Fri, May 7, 2021 at 12:55 PM Andrey Yegorov
> > >>> <an...@datastax.com> wrote:
> > >>> >
> > >>> > I created https://github.com/apache/bookkeeper/pull/2712 with docs
> > and
> > >>> > release notes update for the v 4.14.0
> > >>> > The most interesting part is the release notes:
> > >>> >
> > >>>
> >
> https://github.com/apache/bookkeeper/blob/e3c5994c05c970e6343fa9b43d1e63bac6142e60/site/docs/4.14.0/overview/releaseNotes.md
> > >>> >
> > >>> > Some PRs missed milestones and/or release labels, probably merged
> > >>> manually.
> > >>> > I tracked changelists from git history and updated the
> > >>> labels/milestones.
> > >>> >
> > >>> > I'll start working on the release.
> > >>> >
> > >>> > On Fri, May 7, 2021 at 8:52 AM Andrey Yegorov <
> > >>> andrey.yegorov@datastax.com>
> > >>> > wrote:
> > >>> >
> > >>> > > I added https://github.com/apache/bookkeeper/issues/2711 for the
> > >>> TLS 1.3
> > >>> > > support
> > >>> > > Unless someone objects in the next 30min, I'll merge Lari's PR.
> > >>> > > After that I'll start working on the BK 4.14.0 release.
> > >>> > >
> > >>> > > On Thu, May 6, 2021 at 1:25 AM Lari Hotari <La...@hotari.net>
> > wrote:
> > >>> > >
> > >>> > >> I can confirm that the PR checks pass after excluding TLSv1.3
> from
> > >>> enabled
> > >>> > >> protocols:
> > >>> > >>
> > >>> > >>
> > >>>
> >
> https://github.com/apache/bookkeeper/pull/2696/commits/6003a374d5aec30d7059a21e473ac91417b5cdc3
> > >>> > >>
> > >>> > >> There should be tests for both TLSv1.2 and TLSv1.3 because of
> the
> > >>> > >> differences in TLS handshake described in
> > >>> > >>
> > >>> > >>
> > >>>
> >
> https://urldefense.proofpoint.com/v2/url?u=https-3A__stackoverflow.com_a_62465859&d=DwIFaQ&c=adz96Xi0w1RHqtPMowiL2g&r=0B1UvYMwy7dr9qtqFwQCfxUyrozUgZzbOshynTIaYUY&m=76JE79AuinlMNecD5DDFGgg-jXzCGZEh3PANpQOJUoE&s=iZz_eExfeElZI--ooxMmyMABWjailhDc7rKIAZNg59s&e=
> > >>> > >> .
> > >>> > >>
> > >>> > >> This also impacts some production code in Bookkeeper. The PR
> > already
> > >>> > >> includes a change to catch SSLException instead of
> > >>> SSLHandshakeException (
> > >>> > >>
> > >>> > >>
> > >>>
> >
> https://github.com/apache/bookkeeper/pull/2696/commits/fcbd707a633ed1b8cf8290cb5d70a3070e010196
> > >>> > >> ).
> > >>> > >> TLSv1.3 doesn't throw SSLHandshakeException for certificate
> issues
> > >>> because
> > >>> > >> of the differences in the protocols. This change should work for
> > >>> both
> > >>> > >> protocols, but we should have test coverage to ensure that.
> > >>> > >>
> > >>> > >> TLSv1.3 has been enabled by default since Netty 4.1.52.Final
> (when
> > >>> the JDK
> > >>> > >> contains TLSv1.3). TLSv1.3 support has been available in Java 8
> > >>> since
> > >>> > >> 8u262
> > >>> > >> .
> > >>> > >>
> > >>> > >> One of the remaining problems with TLSv1.3 support in BK is the
> > >>> state
> > >>> > >> machine and TLS counters in PerChannelBookieClient . It doesn't
> > >>> properly
> > >>> > >> model the way TLS 1.3 behaves. Currently there's a counter
> > >>> > >> FAILED_TLS_HANDSHAKE_COUNTER  which is expected to count also
> the
> > >>> > >> certificate issues (code:
> > >>> > >>
> > >>> > >>
> > >>>
> >
> https://github.com/apache/bookkeeper/blob/fcbd707a633ed1b8cf8290cb5d70a3070e010196/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L1535-L1543
> > >>> > >> ). Since TLSv1.3 doesn't detect certificate issues (mutual TLS)
> > >>> during
> > >>> > >> handshake, this counter doesn't count certificate issues.
> > >>> Certificate
> > >>> > >> issues will show up as successfully established connections.
> > >>> > >> The original issue for adding TLS counters was
> > >>> > >> https://github.com/apache/bookkeeper/issues/1103 and PR commit
> > was
> > >>> > >>
> > >>> > >>
> > >>>
> >
> https://github.com/apache/bookkeeper/commit/fa10b7dcd89c40222ba5f30bb60f785bd21669b2
> > >>> > >> .
> > >>> > >>
> > >>> > >> How do we revisit the TLS counter solution for TLSv1.3
> > >>> compatibility? Do
> > >>> > >> we
> > >>> > >> make changes to the code or do we simply skip the test on
> TLSv1.3
> > >>> which
> > >>> > >> ensures that a certificate issue is counted in
> > >>> > >> FAILED_TLS_HANDSHAKE_COUNTER
> > >>> > >> ? Skipping the test for TLSv1.3 would be one option. WDYT?
> > >>> > >>
> > >>> > >> -Lari
> > >>> > >>
> > >>> > >> On Wed, May 5, 2021 at 10:32 PM Andrey Yegorov <
> > >>> > >> andrey.yegorov@datastax.com>
> > >>> > >> wrote:
> > >>> > >>
> > >>> > >> > Lari and I have looked at the Netty upgrade.
> > >>> > >> > There are some test breaks, and so far everything is related
> to
> > >>> behavior
> > >>> > >> > changes related to TLS 1.3, see
> > >>> > >>
> > >>>
> >
> https://urldefense.proofpoint.com/v2/url?u=https-3A__stackoverflow.com_a_62465859&d=DwIFaQ&c=adz96Xi0w1RHqtPMowiL2g&r=0B1UvYMwy7dr9qtqFwQCfxUyrozUgZzbOshynTIaYUY&m=76JE79AuinlMNecD5DDFGgg-jXzCGZEh3PANpQOJUoE&s=iZz_eExfeElZI--ooxMmyMABWjailhDc7rKIAZNg59s&e=
> > >>> > >> > We managed to fix some of the issues
> > >>> > >> > https://github.com/apache/bookkeeper/pull/2696 but "the
> client
> > >>> won't
> > >>> > >> know
> > >>> > >> > whether the server has accepted the certificate or not until
> it
> > >>> next
> > >>> > >> reads
> > >>> > >> > data from the server" complicates things.
> > >>> > >> >
> > >>> > >> > Currently we are considering simply setting "java
> > >>> > >> > -Djdk.tls.client.protocols=TLSv1.2" to unbreak the tests and
> > >>> handling
> > >>> > >> > tls1.3 as a separate work item.
> > >>> > >> >
> > >>> > >> > Lari is planning on spending a little bit more time on this
> > >>> tomorrow
> > >>> > >> (his
> > >>> > >> > tomorrow) to see if there is a better way to address this
> > >>> quickly; we'll
> > >>> > >> > hear more then.
> > >>> > >> >
> > >>> > >> >
> > >>> > >> > On Wed, May 5, 2021 at 9:23 AM Henry Saputra <
> > >>> henry.saputra@gmail.com>
> > >>> > >> > wrote:
> > >>> > >> >
> > >>> > >> > > I am +1 for having next release as 4.14.0
> > >>> > >> > >
> > >>> > >> > > - Henry
> > >>> > >> > >
> > >>> > >> > > On Tue, May 4, 2021 at 2:51 PM Andrey Yegorov <
> > >>> > >> > andrey.yegorov@datastax.com
> > >>> > >> > > >
> > >>> > >> > > wrote:
> > >>> > >> > >
> > >>> > >> > > > Overall +1 for 4.14.0 - the milestone is due May 16th
> > anyway.
> > >>> > >> > > > There is nothing that breaks compatibility with 4.13 so we
> > >>> can skip
> > >>> > >> > > 4.13.1.
> > >>> > >> > > >
> > >>> > >> > > > One thing I'd love to see in 4.14 is
> > >>> > >> > > > https://github.com/apache/bookkeeper/pull/2696 , to fix
> > >>> > >> > > > https://github.com/netty/netty/issues/10986
> > >>> > >> > > > It looks like there are issues with vertx
> > >>> > >> > > >
> > >>> > >>
> > >>>
> https://github.com/apache/bookkeeper/pull/2693#issuecomment-823774769
> > >>> > >> > > > I hope we can upgrade to latest vertx 3.9.7 and netty
> 4.1.60
> > >>> > >> > > >
> > >>> > >> > > > Also https://github.com/apache/bookkeeper/pull/2695
> > upgrades
> > >>> > >> libthrift
> > >>> > >> > > to
> > >>> > >> > > > address security issues.
> > >>> > >> > > >
> > >>> > >> > > > All these PRs are from Lari, I'll follow up with him.
> > >>> > >> > > >
> > >>> > >> > > > I can be a RM if there are no other volunteers.
> > >>> > >> > > >
> > >>> > >> > > >
> > >>> > >> > > > On Tue, May 4, 2021 at 1:57 PM Matteo Merli <
> > >>> mmerli@apache.org>
> > >>> > >> wrote:
> > >>> > >> > > >
> > >>> > >> > > > > +1 We should do 4.14, carrying all the fixes that are
> > there
> > >>> in
> > >>> > >> master
> > >>> > >> > > as
> > >>> > >> > > > > well.
> > >>> > >> > > > >
> > >>> > >> > > > >
> > >>> > >> > > > > --
> > >>> > >> > > > > Matteo Merli
> > >>> > >> > > > > <mm...@apache.org>
> > >>> > >> > > > >
> > >>> > >> > > > > On Tue, May 4, 2021 at 11:57 AM Sijie Guo <
> > >>> guosijie@gmail.com>
> > >>> > >> > wrote:
> > >>> > >> > > > > >
> > >>> > >> > > > > > +1
> > >>> > >> > > > > >
> > >>> > >> > > > > > On Tue, May 4, 2021 at 2:22 AM Yunze Xu
> > >>> > >> > <yzxu@streamnative.io.invalid
> > >>> > >> > > >
> > >>> > >> > > > > > wrote:
> > >>> > >> > > > > >
> > >>> > >> > > > > > > Hello,
> > >>> > >> > > > > > > About 10 days ago I found a heap memory copy problem
> > in
> > >>> Apache
> > >>> > >> > > > Pulsar,
> > >>> > >> > > > > see
> > >>> > >> > > > > > > [1].
> > >>> > >> > > > > > > It’s a problem of BK side because when
> > >>> > >> > `LedgerHandle#asyncAddEntry`
> > >>> > >> > > > > > > accepts a `CompositeByteBuf` or a wrapper, it will
> > >>> finally
> > >>> > >> call
> > >>> > >> > > > > > > `ByteBuf#nioBuffer()`, which would make a heap copy
> > from
> > >>> > >> direct
> > >>> > >> > > > memory.
> > >>> > >> > > > > > > [2] fixed this problem and has been merged for a
> week.
> > >>> > >> > > > > > >
> > >>> > >> > > > > > > Since it has a significant impact on Pulsar, Pulsar
> > side
> > >>> > >> needs a
> > >>> > >> > > new
> > >>> > >> > > > BK
> > >>> > >> > > > > > > release with [2] merged to fix it. Is there any plan
> > to
> > >>> cut a
> > >>> > >> > > 4.13.1
> > >>> > >> > > > > > > release or 4.14.0 release so that we can upgrade the
> > >>> > >> dependency
> > >>> > >> > in
> > >>> > >> > > > > Pulsar?
> > >>> > >> > > > > > >
> > >>> > >> > > > > > > Thanks,
> > >>> > >> > > > > > > Yunze
> > >>> > >> > > > > > >
> > >>> > >> > > > > > > [1] https://github.com/apache/pulsar/pull/10330 <
> > >>> > >> > > > > > > https://github.com/apache/pulsar/pull/10330>
> > >>> > >> > > > > > >
> > >>> > >> > > > > > > [2] https://github.com/apache/bookkeeper/pull/2701
> <
> > >>> > >> > > > > > > https://github.com/apache/bookkeeper/pull/2701>
> > >>> > >> > > > > > >
> > >>> > >> > > > > > >
> > >>> > >> > > > >
> > >>> > >> > > >
> > >>> > >> > > >
> > >>> > >> > > > --
> > >>> > >> > > >
> > >>> > >> > > > --
> > >>> > >> > > > Andrey Yegorov
> > >>> > >> > > >
> > >>> > >> > >
> > >>> > >> >
> > >>> > >> >
> > >>> > >> > --
> > >>> > >> >
> > >>> > >> > --
> > >>> > >> > Andrey Yegorov
> > >>> > >> >
> > >>> > >>
> > >>> > >
> > >>> > >
> > >>> > > --
> > >>> > >
> > >>> > > --
> > >>> > > Andrey Yegorov
> > >>> > >
> > >>> >
> > >>> >
> > >>> > --
> > >>> >
> > >>> > --
> > >>> > Andrey Yegorov
> > >>>
> > >>
> > >>
> > >> --
> > >>
> > >> --
> > >> Andrey Yegorov
> > >>
> > >
> > >
> > > --
> > >
> > > --
> > > Andrey Yegorov
> > >
> >
> >
> > --
> >
> > --
> > Andrey Yegorov
> >
>