[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/669 Merged to master branch. Thanks @ivmaykov ! ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user ivmaykov commented on the issue: https://github.com/apache/zookeeper/pull/669 @anmolnar does anything else need to be done with this PR before it can be merged? ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2603/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user ivmaykov commented on the issue: https://github.com/apache/zookeeper/pull/669 @maoling it will be hard to do perf comparison of netty3 vs netty4 for us because we are currently using the NIO transports, and we don't plan on switching to netty3 in production. Comparing NIO vs netty4 is kind of apples-and-oranges. A quick perf test with this patch showed that NIO to netty4 did not result in any obvious performance regressions, so I think we can say they are at least comparable :) ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2588/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user ivmaykov commented on the issue: https://github.com/apache/zookeeper/pull/669 Rebase, update localAddress after accepting connection and log it ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user maoling commented on the issue: https://github.com/apache/zookeeper/pull/669 find a [blog](https://blog.twitter.com/engineering/en_us/a/2013/netty-4-at-twitter-reduced-gc-overhead.html) about the twitter's practice of moving netty3 to netty4 for Finagle.it couldn't be better to give a benchmark like this: > 5 times less frequent GC pauses: 45.5 vs. 9.2 times/min > 5 times less garbage production: 207.11 vs 41.81 MiB/s ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user ivmaykov commented on the issue: https://github.com/apache/zookeeper/pull/669 @normanmaurer a review from you would be very much appreciated! I bought your book (Netty in Action) which helped me quite a bit :) This is the version of the code we've been testing on a real cluster, so I think it is pretty ready for review. There may be some minor changes still coming, but nothing significant. ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user normanmaurer commented on the issue: https://github.com/apache/zookeeper/pull/669 Also @eolivelli ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user normanmaurer commented on the issue: https://github.com/apache/zookeeper/pull/669 @ivmaykov I would be have to review your code changes once you think these are ready. While I don't know a lot about Zookeeper internals I know a few things about Netty ;) Just ping me please ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2528/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2496/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user ivmaykov commented on the issue: https://github.com/apache/zookeeper/pull/669 Fixed various issues in netty code ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2484/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2485/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2483/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user ivmaykov commented on the issue: https://github.com/apache/zookeeper/pull/669 Cleaner Epoll/Nio selection code ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user ivmaykov commented on the issue: https://github.com/apache/zookeeper/pull/669 @eolivelli use `voidPromise()` to avoid allocations when writing to channel ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2482/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user ivmaykov commented on the issue: https://github.com/apache/zookeeper/pull/669 @eolivelli changes you requested: - use Epoll if available - if (LOG.isDebugEnabled()) around complex LOG.debug() statements - use netty-all artifact Haven't looked into voidPromise() yet. ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/669 @maoling there is no generic benchmarks. For instance in Bookkeeper we switched to Netty 4 and now we are able to leverage all the cool stuff about memory and so we are able to be more efficient. Overall by default Netty4 will prefer using off heap memory, this is about being able to do as few as possible memory copies while passing data to the SO. The simple switch may or may not make overall performance better or even worse. There will be knobs to tune. Netty 3 is almost obsolete and IMHO it is better to stay on the latest and greatest. Netty4 project is very active. Another topic will be about refcounting, Netty4 enables even Java programs to work easy with direct memory and with heap memory and provides very efficient ways to pool memory and java objects (see the Recycler facility). So I think this is only the beginning of this journey ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user maoling commented on the issue: https://github.com/apache/zookeeper/pull/669 - where can we find a benchmark comparing netty4 vs netty3?I am testing the perfermance - saw many use cases facing memory leak when migrate netty3 to netty4 for misunderstading the netty4 new desigin,worry about this. ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2480/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2479/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/669 @ivmaykov I answered to all of your comments. I expect this to be the very first step. Coming with Netty 4 it is more easier to reduce memory allocations, expecially on the hot paths. I expect in the future that we will introduce a lot of tricks and techniques to leverage all the potential of Netty ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user ivmaykov commented on the issue: https://github.com/apache/zookeeper/pull/669 @eolivelli thanks so much for the review! See my responses inline. ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user enixon commented on the issue: https://github.com/apache/zookeeper/pull/669 @eolivelli , good find with EPoll. :) When @ivmaykov first mentioned using EPoll to me as a potential optimization, I recommended leaving it for later so we would do the reviewers a favor and keep the complexity of this pull request relatively low. We do think it's the right implementation to use here, the only question is where/when to make that contribution. ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2473/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2463/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2444/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2445/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2443/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user enixon commented on the issue: https://github.com/apache/zookeeper/pull/669 @ivmaykov (or anyone with a stack overflow account, really), do you want to tag a pointer to this PR with 'netty' on stack overflow (as per https://netty.io/community.html) and see if any of the project experts want to weigh in on this port? ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2442/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2441/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2439/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/669 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2438/ ---
[GitHub] zookeeper issue #669: ZOOKEEPER-3152: Port ZK netty stack to netty4
Github user ivmaykov commented on the issue: https://github.com/apache/zookeeper/pull/669 @dain take a look at the changes I made to airlift's test ByteBuf allocator. With these changes, we (sometimes) get leak details printed to stderr if a ByteBuf leaks, before the test crashes. Feel free to incorporate the approach into airlift if you want. ---
