[GitHub] drill pull request #1008: drill-5890: Fixed a file descriptor leak in Drill'...

[sachouche <gi...@git.apache.org>]

GitHub user sachouche opened a pull request:

    https://github.com/apache/drill/pull/1008

    drill-5890: Fixed a file descriptor leak in Drill's test-suite

    Problem Description
    - The Drill test-suite uses two surefire processes to run tests
    - This has the advantage of avoiding class reloading if the JVM exited after running a test class
    - The side effect of this approach, is that resource leaks could be problematic
    - When running the Drill's test-suite on MacOS (Sierra) my tests failed with a max FD descriptors reached
    - Had to increase the maximum number of open FDs for the whole machine and per process
    - The process is described in the following [link](https://superuser.com/questions/302754/increase-the-maximum-number-of-open-file-descriptors-in-snow-leopard)
    - Two limit files "limit.maxfiles.plist" and "limit.maxproc.plist" have to be created under "/Library/LaunchDaemons"
    - Originally, I had to set the maximum number of FDs per process to a large value (100,000 and the system to 200,000) for the tests to succeed
    
    FD Leak Cause
    Debugging the Drill test suite, it was noticed 
    - A base class BaseTestQuery has a @BeforeClass and @AfterClass TestNG tags 
    - This means that each Drill test class extending from BaseTestQuery will have a setup method called before any tests are executed and a cleanup method invoked when all the tests are done (or a fatal error in between)
    - The OpenClient() method was starting a DrillBit and creating a client connection to it
    - DrillBit's BootStrapContext class was initializing two Netty EventLoopGroup objects which internally opened 20 FDs each
    - It was noticed that one of them was not getting de-initialized
    
    Fix
    - Added logic within the BootStrapContext object to shutdown the EventLoopGroup objects if they have been already shutdown (and are not in the process of being shutdown)
    - The fix tries to shut both objects because the container class should ideally manage the lifecycle of its objects; at least, the code should clearly articulate lifecycle management responsibilities to avoid leaks
    - Used the "shutdownGracefully" method since it was a) already used by our code and b) is advertised to have sensible timeout values
    - The added shutdown calls are being invoked only when consumer objects have been also shutdown
    - Running the tests show that the number of FDs per surefire process doesn't extend beyond few hundreds (majority created for JAR files loading)


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

    $ git pull https://github.com/sachouche/drill drill-5890

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

    https://github.com/apache/drill/pull/1008.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 #1008
    
----
commit 5f8bad865a78ab265015ca21184d6c59e22d1c95
Author: Salim Achouche <sa...@gmail.com>
Date:   2017-10-24T17:12:19Z

    drill-5890: Fixed a file descriptor leak in Drill's test-suite

----


---

[GitHub] drill issue #1008: drill-5890: Fixed a file descriptor leak in Drill's test-...

[sachouche <gi...@git.apache.org>]

Github user sachouche commented on the issue:

    https://github.com/apache/drill/pull/1008
  
    @parthchandra can you please review this change? thanks!


---

[GitHub] drill issue #1008: drill-5890: Fixed a file descriptor leak in Drill's test-...

[parthchandra <gi...@git.apache.org>]

Github user parthchandra commented on the issue:

    https://github.com/apache/drill/pull/1008
  
    +1. 
    Great catch Salim. 


---

[GitHub] drill pull request #1008: drill-5890: Fixed a file descriptor leak in Drill'...

[asfgit <gi...@git.apache.org>]

Github user asfgit closed the pull request at:

    https://github.com/apache/drill/pull/1008


---