+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 <an...@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? > > Andor > > > > > > On 2019. Jan 4., at 16:27, Enrico Olivelli <eolive...@gmail.com> wrote: > > > > Il giorno ven 4 gen 2019 alle ore 14:23 Andor Molnar > > <an...@apache.org <mailto:an...@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 > >
