[GitHub] incubator-flink pull request: [FLINK-930] Netty Initialization is ...

[zentol]

GitHub user uce opened a pull request:

    https://github.com/apache/incubator-flink/pull/19

    [FLINK-930] Netty Initialization is sometimes very slow

    This is [FLINK-930](https://issues.apache.org/jira/browse/FLINK-930).
   
    The issue with the Netty warning was reported in netty/netty#2412 and has been fixed in `4.0.19.Final`. I couldn't reproduce the reported problem (with and without the changes in this PR).
   
    This PR contains the following changes:
    1. Update Netty version from `4.0.19.Final` to `4.0.20.Final` and
    2. Introduce `ExecutionMode` for TaskManagers, where in `LOCAL` mode the `NetworkConnectionManager` is not started (e.g. no ServerSocket binding etc.)
   
    The 2. change speeds up the startup and shutdown time of the TaskManagers noticeably and solves/circumvents the reported problem with each test taking ~ 3 seconds for startup/shutdown.
   


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/uce/incubator-flink FLINK-930

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-flink/pull/19.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #19
   
----
commit a0d3530aa137290bf2d690339bf028f272c12129
Author: uce <[hidden email]>
Date:   2014-06-13T08:41:32Z

    [FLINK-930] Netty Initialization is sometimes very slow

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user uce commented on the pull request:

    https://github.com/apache/incubator-flink/pull/19#issuecomment-46251209
 
    Can someone please review and merge? Travis is running consistently out of time atm and this PR should "fix it". We can then further monitor the Netty ThreadLocalRandom warning.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user rmetzger commented on a diff in the pull request:

    https://github.com/apache/incubator-flink/pull/19#discussion_r13845277
 
    --- Diff: stratosphere-runtime/src/main/java/eu/stratosphere/runtime/io/network/ChannelManager.java ---
    @@ -99,8 +95,13 @@ public ChannelManager(ChannelLookupProtocol channelLookupService, InstanceConnec
      this.discardBufferPool = new DiscardBufferPool();
      }
     
    - public void shutdown() {
    - this.nettyConnectionManager.shutdown();
    + public void shutdown()  {
    + try {
    + this.networkConnectionManager.shutdown();
    + } catch (IOException e) {
    + LOG.warn("NetworkConnectionManager did not shutdown properly.");
    --- End diff --
   
    I think we should log the exception as well here (I guess the error is unlikely and needs review by the user)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user rmetzger commented on a diff in the pull request:

    https://github.com/apache/incubator-flink/pull/19#discussion_r13845310
 
    --- Diff: stratosphere-runtime/src/main/java/eu/stratosphere/runtime/io/network/netty/NettyConnectionManager.java ---
    @@ -108,9 +130,9 @@ public void initChannel(SocketChannel channel) throws Exception {
      .option(ChannelOption.SO_KEEPALIVE, true);
     
      try {
    - this.in.bind().sync();
    + in.bind().sync();
      } catch (InterruptedException e) {
    - throw new RuntimeException("Could not bind server socket for incoming connections.");
    + throw new IOException("Interrupted while trying to bind server socket.");
    --- End diff --
   
    I would also forward the exception here


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user uce commented on the pull request:

    https://github.com/apache/incubator-flink/pull/19#issuecomment-46284531
 
    I agree with both your points and have addressed them. The thrown exceptions are rethrown as IOExceptions up to the TaskManager's shutdown method, where it is logged. I decided against changing the signature of the TaskManager's shutdown method to rethrow the Exception as I think this should go hand in hand with changes to the other component shutdowns as well.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user uce commented on the pull request:

    https://github.com/apache/incubator-flink/pull/19#issuecomment-46297333
 
    Nice to see it green again: https://travis-ci.org/uce/incubator-flink/builds/27754618


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user StephanEwen commented on the pull request:

    https://github.com/apache/incubator-flink/pull/19#issuecomment-46299700
 
    Looks good to me. Will merge.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[zentol]

Github user asfgit closed the pull request at:

    https://github.com/apache/incubator-flink/pull/19


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---