zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Enrico Olivelli <eolive...@gmail.com>
Subject Re: JAVA 11 build is broken on 3.5
Date Fri, 04 Jan 2019 17:08:12 GMT
Il giorno ven 4 gen 2019 alle ore 17:04 Norbert Kalmar
<nkalmar@cloudera.com.invalid> ha scritto:
>
> +1 for Netty 4 in 3.5
>
> Pretty much all the pros and cons has been said before me.
> I would only add that this is not a new functionality that we wan't to
> backport. It's a criticall(ish?) bugfix, which requires quite a bit of
> change unfortunately.
>
> Regards,
> Norbert
>
> On Fri, Jan 4, 2019 at 4:36 PM Andor Molnar <andor@apache.org> wrote:
>
> > What are those patches exactly?
> >
> > Comparing the ported version of 3.5 with master I’ve only found 2 patches
> > which are missing:
> >
> > ZOOKEEPER-3146 Limit the maximum client connections per IP in
> > NettyServerCnxnFactory
> > ZOOKEEPER-3177 Refactor request throttle logic in NIO and Netty to keep
> > the same behavior and make the code easier to maintain
> >
> > None of them are critical I would say.
> > Is there anything else I’m missing?

I have compared the histories and I think Andor you are right.

master:
https://github.com/apache/zookeeper/commits/master

branch-3.5:
https://github.com/apache/zookeeper/commits/branch-3.5

Sorry I was thinking to the amount of patches related to TLS stuff ,
but they are not related to Netty 4.

What about branch 3.4 ?
We don't have reconfig but the case "Start embedded ZK, stop it and
try to restart on the same port" should apply as well.

Enrico

> >
> > Andor
> >
> >
> >
> >
> > > On 2019. Jan 4., at 16:27, Enrico Olivelli <eolivelli@gmail.com> wrote:
> > >
> > > Il giorno ven 4 gen 2019 alle ore 14:23 Andor Molnar
> > > <andor@apache.org <mailto:andor@apache.org>> ha scritto:
> > >>
> > >> Hi team / Enrico,
> > >>
> > >> I’d like to get feedback from the community on the following patch
> > (moving the discussion from GitHub to here):
> > >>
> > >> https://issues.apache.org/jira/browse/ZOOKEEPER-3204 <
> > https://issues.apache.org/jira/browse/ZOOKEEPER-3204>
> > >> https://github.com/apache/zookeeper/pull/753 <
> > https://github.com/apache/zookeeper/pull/753>
> > >>
> > >> In a nutshell: looks like that Netty 3.10 is broken under Java 11: it
> > doesn’t properly close the underlying socket (probably not closing the
> > registered NIO selectors) and reconfig tests are unable to re-bind the
> > ports. This problem is similar that we already fixed in NIO with the
> > following patch:
> > >>
> > https://github.com/apache/zookeeper/commit/c3babb94275ad667dc71c10dcb08a383a3c154c2
> > <
> > https://github.com/apache/zookeeper/commit/c3babb94275ad667dc71c10dcb08a383a3c154c2
> > >
> > >> The problem doesn’t show up on trunk which has been recently upgraded
> > to Netty 4.
> > >>
> > >> Repro:
> > >> - Start embedded ZK, stop it and try to restart on the same port, or
> > >> - Start normal ensemble and reconfig to use different (client) port.
> > Then reconfig back to the original port which should fail. (that’s the
> > scenario which is covered in ReconfigTest)
> > >>
> > >> I created the above patch (#753) to backport Netty 4 upgrade to 3.5 and
> > it fixes the problem with Java 11 (it doesn’t cause regression in the
> > pre-commit build either), but Enrico is having concerns about making such
> > big change before the release.
> > >>
> > >> I tend to agree, but let’s see what are the options.
> > >>
> > >> Thoughts:
> > >> - Do we have to fix this? - Yes. Java 11 is LTS and I the bug is
> > critical.
> > >> - Can we fix Netty 3? - Maybe. Let’s say we find the bug in Netty 3,
> > what can we do?
> > >>       a) We cannot workaround from ZooKeeper itself and have to submit
> > a pull request for Netty. I think it’s quite unlikely that they will accept
> > the change given it’s not a security bug, but even if they did, only the
> > upgraded version of Netty 3 would work properly with ZooKeeper. Err.
> > >>       b) We can workaround it from ZooKeeper: that could be option #1,
> > but I have a strong feeling about it’s not going to be the case.
> > >> - Shall we upgrade to Netty 4? - this is option #2
> > >>
> > >> Please share your thoughts, maybe you know about an option #3.
> > >
> > > Thank you Andor
> > >
> > > I have thought more about this problem, and I have checked that Netty
> > > 3 is really dead/unmantained (last release in 2016).
> > > If I understand correctly there is no easy workaround (nothing without
> > > hacking Netty 3 internals)
> > >
> > > As soon as we will declare 3.5.5 "stable" the world will hopefully
> > > abandon 3.4 and switch to 3.5 + Netty (because of SSL support).
> > > The network stack is very important so it is better to have Netty 4 as
> > > foundation, I am thinking about security issues, we won't make an
> > > "hotfix" release with the switch to Netty 4 because there is a bad bug
> > > in Netty 3.
> > > So better to switch now.
> > >
> > > But Facebooks friends, expecially @ivmaykov did a lot of bugfixes
> > > around Netty on master branch, we must be sure that what we are
> > > delivering in 3.5.5 is stable.
> > >
> > > We will also have to state clearly in the "release notes" that Netty
> > > version is changed, as this may have a non trivial impact to memory
> > > usage (i.e. Netty 4 uses more Direct memory by default)
> > >
> > > So to recap my final opinion: +1 to switch to Netty 4 if we take care
> > > of port all of the fixes around Netty 4 from master branch and we
> > > state the switch clearly in the release notes
> > >
> > > Enrico
> > >
> > >>
> > >> Regards,
> > >> Andor
> >
> >

Mime
View raw message