Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-alpha
    • Fix Version/s: 3.0.0, 2.0.2-alpha
    • Component/s: conf
    • Labels:
      None

      Description

      The newly introduced CACHE_CLASSES leaks class loaders causing associated classes to not be reclaimed.

      One solution is to remove the cache itself since each class loader implementation caches the classes it loads automatically and preventing an exception from being raised is just a micro-optimization that, as one can tell, causes bugs instead of improving anything.
      In fact, I would argue in a highly-concurrent environment, the weakhashmap synchronization/lookup probably costs more then creating the exception itself.

      Another is to prevent the leak from occurring, by inserting the loadedclass into the WeakHashMap wrapped in a WeakReference. Otherwise the class has a strong reference to its classloader (the key) meaning neither gets GC'ed.
      And since the cache_class is static, even if the originating Configuration instance gets GC'ed, its classloader won't.

      1. HADOOP-8632-trunk-no-tabs.patch
        4 kB
        Costin Leau
      2. HADOOP-8632-trunk.patch
        4 kB
        Costin Leau
      3. HADOOP-8632.patch
        4 kB
        Costin Leau
      4. 0001-wrapping-classes-with-WeakRefs-in-CLASS_CACHE.patch
        4 kB
        Costin Leau

        Issue Links

          Activity

          Hide
          Robert Joseph Evans added a comment -

          Before we do anything we probably want to set up a micro-benchmark to validate any changes that happen. I personally don't have much of a problem leaking classes in Hadoop itself so long as we document that it might happen. We typically don't use one class and then drop it. In almost all cases we will load a class that is used throughout the life of a process. I am not sure about other projects that also use Configuration.

          Show
          Robert Joseph Evans added a comment - Before we do anything we probably want to set up a micro-benchmark to validate any changes that happen. I personally don't have much of a problem leaking classes in Hadoop itself so long as we document that it might happen. We typically don't use one class and then drop it. In almost all cases we will load a class that is used throughout the life of a process. I am not sure about other projects that also use Configuration.
          Hide
          Todd Lipcon added a comment -

          Hi Costin. We added the CACHE_CLASSES member in HADOOP-6133 to solve a performance regression which we saw in practice. HADOOP-6502 also talks about another case where the performance of this code was critical. So I don't think removing the cache is a good idea.

          That said, I do think changing the cache to have weak values as well as weak keys makes sense. Guava's MapMaker has a nice utility to do this, or we could simply change the class values to be Map<String, WeakReference<Class<?>>>.

          Show
          Todd Lipcon added a comment - Hi Costin. We added the CACHE_CLASSES member in HADOOP-6133 to solve a performance regression which we saw in practice. HADOOP-6502 also talks about another case where the performance of this code was critical. So I don't think removing the cache is a good idea. That said, I do think changing the cache to have weak values as well as weak keys makes sense. Guava's MapMaker has a nice utility to do this, or we could simply change the class values to be Map<String, WeakReference<Class<?>>>.
          Hide
          Costin Leau added a comment -

          @Robert

          My issue is not with the cache itself but with the leakage. If a client submits several big jobs, she has to either launch a new JVM for each submission or somehow patch the leak from outside. Or face OOM.
          Addressing this in the framework directly obviously is much better.

          @Todd
          Wrapping the value with a WeakReference probably it's the easiest solution since it doesn't introduce a new library dependency. It can later be upgraded to MapMaker if the pattern occurs often.

          Show
          Costin Leau added a comment - @Robert My issue is not with the cache itself but with the leakage. If a client submits several big jobs, she has to either launch a new JVM for each submission or somehow patch the leak from outside. Or face OOM. Addressing this in the framework directly obviously is much better. @Todd Wrapping the value with a WeakReference probably it's the easiest solution since it doesn't introduce a new library dependency. It can later be upgraded to MapMaker if the pattern occurs often.
          Hide
          Robert Joseph Evans added a comment -

          Costin,

          I understand your issue more fully now, and I am fine if you want to add in WeakReferences to the ClassLoaders. If you have a patch for this leak, I would be happy to review it.

          Show
          Robert Joseph Evans added a comment - Costin, I understand your issue more fully now, and I am fine if you want to add in WeakReferences to the ClassLoaders. If you have a patch for this leak, I would be happy to review it.
          Hide
          Costin Leau added a comment -

          I've attached my patch. I picked it up from my fork on GitHub (of Hadoop Commons) - based it on hadoop-2.0.1 branch.
          See the code here: https://github.com/costin/hadoop-common/commit/57d9df37e600dd588a737d67b271657561ebfea2

          Show
          Costin Leau added a comment - I've attached my patch. I picked it up from my fork on GitHub (of Hadoop Commons) - based it on hadoop-2.0.1 branch. See the code here: https://github.com/costin/hadoop-common/commit/57d9df37e600dd588a737d67b271657561ebfea2
          Hide
          Costin Leau added a comment -

          git patch

          Show
          Costin Leau added a comment - git patch
          Hide
          Costin Leau added a comment -

          By the way, in the same vein, ReflectionUtils#CONSTRUCTOR_CACHE also leaks classes (see HADOOP-8605 - I'm happy to fix that as well if you want).

          Show
          Costin Leau added a comment - By the way, in the same vein, ReflectionUtils#CONSTRUCTOR_CACHE also leaks classes (see HADOOP-8605 - I'm happy to fix that as well if you want).
          Hide
          Robert Joseph Evans added a comment -

          Kicking Jenkins so it will test the patch.

          Show
          Robert Joseph Evans added a comment - Kicking Jenkins so it will test the patch.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12540677/0001-wrapping-classes-with-WeakRefs-in-CLASS_CACHE.patch
          against trunk revision .

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1286//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12540677/0001-wrapping-classes-with-WeakRefs-in-CLASS_CACHE.patch against trunk revision . -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1286//console This message is automatically generated.
          Hide
          Costin Leau added a comment -

          attaching result of:
          git diff --no-prefix fa23683720 57d9df3

          Show
          Costin Leau added a comment - attaching result of: git diff --no-prefix fa23683720 57d9df3
          Hide
          Costin Leau added a comment -

          Since the previous patch (the standard git/GitHub patch [1]) didn't apply correctly, I've attached the git diff result as specified on the Hadoop wiki [2]

          [1] https://github.com/costin/hadoop-common/commit/57d9df37e600dd588a737d67b271657561ebfea2.patch
          [2] http://wiki.apache.org/hadoop/GitAndHadoop

          Show
          Costin Leau added a comment - Since the previous patch (the standard git/GitHub patch [1] ) didn't apply correctly, I've attached the git diff result as specified on the Hadoop wiki [2] [1] https://github.com/costin/hadoop-common/commit/57d9df37e600dd588a737d67b271657561ebfea2.patch [2] http://wiki.apache.org/hadoop/GitAndHadoop
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12540887/HADOOP-8632.patch
          against trunk revision .

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1295//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12540887/HADOOP-8632.patch against trunk revision . -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1295//console This message is automatically generated.
          Hide
          Costin Leau added a comment -

          Guys I'm not sure why the patch doesn't apply - I've followed verbatim the instructions from the wiki. Any ideas on what's missing?

          Show
          Costin Leau added a comment - Guys I'm not sure why the patch doesn't apply - I've followed verbatim the instructions from the wiki. Any ideas on what's missing?
          Hide
          Robert Joseph Evans added a comment -

          When I try to apply the patch to trunk I get

          $ patch -p 0 < HADOOP-8632.patch 
          patching file hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java
          Hunk #3 succeeded at 1532 (offset 55 lines).
          patching file hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java
          Hunk #2 FAILED at 1044.
          1 out of 2 hunks FAILED -- saving rejects to file hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java.rej
          

          You may want to try upmerging the patch to trunk.

          Show
          Robert Joseph Evans added a comment - When I try to apply the patch to trunk I get $ patch -p 0 < HADOOP-8632.patch patching file hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java Hunk #3 succeeded at 1532 (offset 55 lines). patching file hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java Hunk #2 FAILED at 1044. 1 out of 2 hunks FAILED -- saving rejects to file hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java.rej You may want to try upmerging the patch to trunk.
          Hide
          Costin Leau added a comment -

          Attaching the patch against the trunk (the previous patch as mentioned, was against branch-2.1.0-alpha).
          Ran
          git diff --no-prefix 7a3427de68 518c39814e

          The github link is: http://j.mp/Nor44u

          Did a cherry pick of the commit against 2.1.0-alpha.

          Show
          Costin Leau added a comment - Attaching the patch against the trunk (the previous patch as mentioned, was against branch-2.1.0-alpha). Ran git diff --no-prefix 7a3427de68 518c39814e The github link is: http://j.mp/Nor44u Did a cherry pick of the commit against 2.1.0-alpha.
          Hide
          Costin Leau added a comment -

          By the way, tried patch -p 0 < HADOOP-8632-trunk.patch and it applied cleanly against the trunk.

          Show
          Costin Leau added a comment - By the way, tried patch -p 0 < HADOOP-8632 -trunk.patch and it applied cleanly against the trunk.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12541186/HADOOP-8632-trunk.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.ha.TestZKFailoverController

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1312//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1312//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12541186/HADOOP-8632-trunk.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ha.TestZKFailoverController +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1312//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1312//console This message is automatically generated.
          Hide
          Costin Leau added a comment -

          The ZK failures seem to be caused by the CI host, independently from this patch.

          Show
          Costin Leau added a comment - The ZK failures seem to be caused by the CI host, independently from this patch.
          Hide
          Robert Joseph Evans added a comment -

          The code looks good and the existing tests all seem to pass. Please remove the tabs from your patch, our coding standard requires the use of spaces instead, and then I am a +1.

          Show
          Robert Joseph Evans added a comment - The code looks good and the existing tests all seem to pass. Please remove the tabs from your patch, our coding standard requires the use of spaces instead, and then I am a +1.
          Hide
          Costin Leau added a comment -

          the patch w/o any tabs.

          Show
          Costin Leau added a comment - the patch w/o any tabs.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12542100/HADOOP-8632-trunk-no-tabs.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1351//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1351//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12542100/HADOOP-8632-trunk-no-tabs.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1351//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1351//console This message is automatically generated.
          Hide
          Robert Joseph Evans added a comment -

          Thanks Constin,

          I merged this into trunk, and branch-2. If you wanted it in another release please add a comment to indicate it.

          Show
          Robert Joseph Evans added a comment - Thanks Constin, I merged this into trunk, and branch-2. If you wanted it in another release please add a comment to indicate it.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2626 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2626/)
          HADOOP-8632. Configuration leaking class-loaders (Costin Leau via bobby) (Revision 1376543)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376543
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2626 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2626/ ) HADOOP-8632 . Configuration leaking class-loaders (Costin Leau via bobby) (Revision 1376543) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376543 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2690 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2690/)
          HADOOP-8632. Configuration leaking class-loaders (Costin Leau via bobby) (Revision 1376543)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376543
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2690 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2690/ ) HADOOP-8632 . Configuration leaking class-loaders (Costin Leau via bobby) (Revision 1376543) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376543 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2654 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2654/)
          HADOOP-8632. Configuration leaking class-loaders (Costin Leau via bobby) (Revision 1376543)

          Result = FAILURE
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376543
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2654 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2654/ ) HADOOP-8632 . Configuration leaking class-loaders (Costin Leau via bobby) (Revision 1376543) Result = FAILURE bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376543 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1144 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1144/)
          HADOOP-8632. Configuration leaking class-loaders (Costin Leau via bobby) (Revision 1376543)

          Result = FAILURE
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376543
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1144 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1144/ ) HADOOP-8632 . Configuration leaking class-loaders (Costin Leau via bobby) (Revision 1376543) Result = FAILURE bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376543 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1175 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1175/)
          HADOOP-8632. Configuration leaking class-loaders (Costin Leau via bobby) (Revision 1376543)

          Result = FAILURE
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376543
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1175 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1175/ ) HADOOP-8632 . Configuration leaking class-loaders (Costin Leau via bobby) (Revision 1376543) Result = FAILURE bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1376543 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java
          Hide
          Suresh Srinivas added a comment -

          Costin, I added you as a contributor to Hadoop and assigned this jira to you. Thanks for contributing to Hadoop.

          Show
          Suresh Srinivas added a comment - Costin, I added you as a contributor to Hadoop and assigned this jira to you. Thanks for contributing to Hadoop.
          Hide
          Costin Leau added a comment -

          Thanks!

          Show
          Costin Leau added a comment - Thanks!

            People

            • Assignee:
              Costin Leau
              Reporter:
              Costin Leau
            • Votes:
              0 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development