Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-5957

AM throws ClassNotFoundException with job classloader enabled if custom output format/committer is used

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.0
    • Fix Version/s: 2.6.0
    • Component/s: None
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      With the job classloader enabled, the MR AM throws ClassNotFoundException if a custom output format class is specified.

      org.apache.hadoop.yarn.exceptions.YarnRuntimeException: java.lang.RuntimeException: java.lang.ClassNotFoundException: Class com.foo.test.TestOutputFormat not found
      	at org.apache.hadoop.mapreduce.v2.app.MRAppMaster.createOutputCommitter(MRAppMaster.java:473)
      	at org.apache.hadoop.mapreduce.v2.app.MRAppMaster.serviceInit(MRAppMaster.java:374)
      	at org.apache.hadoop.service.AbstractService.init(AbstractService.java:163)
      	at org.apache.hadoop.mapreduce.v2.app.MRAppMaster$1.run(MRAppMaster.java:1459)
      	at java.security.AccessController.doPrivileged(Native Method)
      	at javax.security.auth.Subject.doAs(Subject.java:415)
      	at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1548)
      	at org.apache.hadoop.mapreduce.v2.app.MRAppMaster.initAndStartAppMaster(MRAppMaster.java:1456)
      	at org.apache.hadoop.mapreduce.v2.app.MRAppMaster.main(MRAppMaster.java:1389)
      Caused by: java.lang.RuntimeException: java.lang.ClassNotFoundException: Class com.foo.test.TestOutputFormat not found
      	at org.apache.hadoop.conf.Configuration.getClass(Configuration.java:1895)
      	at org.apache.hadoop.mapreduce.task.JobContextImpl.getOutputFormatClass(JobContextImpl.java:222)
      	at org.apache.hadoop.mapreduce.v2.app.MRAppMaster.createOutputCommitter(MRAppMaster.java:469)
      	... 8 more
      Caused by: java.lang.ClassNotFoundException: Class com.foo.test.TestOutputFormat not found
      	at org.apache.hadoop.conf.Configuration.getClassByName(Configuration.java:1801)
      	at org.apache.hadoop.conf.Configuration.getClass(Configuration.java:1893)
      	... 10 more
      
      1. MAPREDUCE-5957.branch-2.patch
        26 kB
        Jason Lowe
      2. MAPREDUCE-5957.patch
        28 kB
        Sangjin Lee
      3. MAPREDUCE-5957.patch
        28 kB
        Sangjin Lee
      4. MAPREDUCE-5957.patch
        26 kB
        Sangjin Lee
      5. MAPREDUCE-5957.patch
        20 kB
        Sangjin Lee
      6. MAPREDUCE-5957.patch
        13 kB
        Sangjin Lee
      7. MAPREDUCE-5957.patch
        6 kB
        Sangjin Lee

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1839 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1839/)
          MAPREDUCE-5957. AM throws ClassNotFoundException with job classloader enabled if custom output format/committer is used. Contributed by Sangjin Lee (jlowe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1612358)

          • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/commit/CommitterEventHandler.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/v2/TestMRJobs.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1839 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1839/ ) MAPREDUCE-5957 . AM throws ClassNotFoundException with job classloader enabled if custom output format/committer is used. Contributed by Sangjin Lee (jlowe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1612358 ) /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/commit/CommitterEventHandler.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/v2/TestMRJobs.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #1812 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1812/)
          MAPREDUCE-5957. AM throws ClassNotFoundException with job classloader enabled if custom output format/committer is used. Contributed by Sangjin Lee (jlowe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1612358)

          • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/commit/CommitterEventHandler.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/v2/TestMRJobs.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #1812 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1812/ ) MAPREDUCE-5957 . AM throws ClassNotFoundException with job classloader enabled if custom output format/committer is used. Contributed by Sangjin Lee (jlowe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1612358 ) /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/commit/CommitterEventHandler.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/v2/TestMRJobs.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #620 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/620/)
          MAPREDUCE-5957. AM throws ClassNotFoundException with job classloader enabled if custom output format/committer is used. Contributed by Sangjin Lee (jlowe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1612358)

          • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/commit/CommitterEventHandler.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/v2/TestMRJobs.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #620 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/620/ ) MAPREDUCE-5957 . AM throws ClassNotFoundException with job classloader enabled if custom output format/committer is used. Contributed by Sangjin Lee (jlowe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1612358 ) /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/commit/CommitterEventHandler.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/v2/TestMRJobs.java
          Hide
          Sangjin Lee added a comment -

          Thanks Jason! Confirming that the only difference for branch-2 is the code for the preemption policy which branch-2 does not have anyway (sorry should have noticed it myself).

          Show
          Sangjin Lee added a comment - Thanks Jason! Confirming that the only difference for branch-2 is the code for the preemption policy which branch-2 does not have anyway (sorry should have noticed it myself).
          Hide
          Jason Lowe added a comment -

          Thanks, Sangjin! I committed this to trunk and branch-2.

          Show
          Jason Lowe added a comment - Thanks, Sangjin! I committed this to trunk and branch-2.
          Hide
          Jason Lowe added a comment -

          Patch had a minor conflict with branch-2, so I'm uploading the patch I'm committing to branch-2 for reference.

          Show
          Jason Lowe added a comment - Patch had a minor conflict with branch-2, so I'm uploading the patch I'm committing to branch-2 for reference.
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #5923 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5923/)
          MAPREDUCE-5957. AM throws ClassNotFoundException with job classloader enabled if custom output format/committer is used. Contributed by Sangjin Lee (jlowe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1612358)

          • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/commit/CommitterEventHandler.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/v2/TestMRJobs.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #5923 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5923/ ) MAPREDUCE-5957 . AM throws ClassNotFoundException with job classloader enabled if custom output format/committer is used. Contributed by Sangjin Lee (jlowe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1612358 ) /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/commit/CommitterEventHandler.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/v2/TestMRJobs.java
          Hide
          Sangjin Lee added a comment -

          Thanks Jason for valuable feedback and committing the patch! Greatly appreciated.

          Show
          Sangjin Lee added a comment - Thanks Jason for valuable feedback and committing the patch! Greatly appreciated.
          Hide
          Jason Lowe added a comment -

          +1 lgtm. Committing this.

          Show
          Jason Lowe added a comment - +1 lgtm. Committing this.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12656274/MAPREDUCE-5957.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. There were no new javadoc warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient.

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

          Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4747//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4747//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/12656274/MAPREDUCE-5957.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 . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4747//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4747//console This message is automatically generated.
          Hide
          Sangjin Lee added a comment -

          Fixed the javadoc.

          Show
          Sangjin Lee added a comment - Fixed the javadoc.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12656184/MAPREDUCE-5957.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. There were no new javadoc warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient.

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

          Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4746//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4746//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/12656184/MAPREDUCE-5957.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 . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4746//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4746//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

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

          -1 patch. Trunk compilation may be broken.

          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4745//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/12656184/MAPREDUCE-5957.patch against trunk revision . -1 patch . Trunk compilation may be broken. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4745//console This message is automatically generated.
          Hide
          Sangjin Lee added a comment -

          Do you think it's worth adding a wrapper form that can throw Exception (or maybe just IOException)?

          Sure. I'll add an overloaded method that can throw an IOException. I did notice that when I was playing with the wrapper method, and was thinking about adding another version.

          Is there a reason the init transition calls MRAppMaster.createJobClassloader rather than having MRAppMaster.serviceInit do it?

          Nothing other than that I wanted to create it as early as possible. There is a fair amount of code between the beginning of that method and serviceInit(). But I think it is fine to do it inside MRAppMaster.serviceInit(). I'll change that.

          it would be nice to clean that up by either removing the argument or use it rather than the committer member.

          I'd be happy to do it. I'll fix that too.

          Show
          Sangjin Lee added a comment - Do you think it's worth adding a wrapper form that can throw Exception (or maybe just IOException)? Sure. I'll add an overloaded method that can throw an IOException. I did notice that when I was playing with the wrapper method, and was thinking about adding another version. Is there a reason the init transition calls MRAppMaster.createJobClassloader rather than having MRAppMaster.serviceInit do it? Nothing other than that I wanted to create it as early as possible. There is a fair amount of code between the beginning of that method and serviceInit(). But I think it is fine to do it inside MRAppMaster.serviceInit(). I'll change that. it would be nice to clean that up by either removing the argument or use it rather than the committer member. I'd be happy to do it. I'll fix that too.
          Hide
          Jason Lowe added a comment -

          Thanks for updating the patch. I do think the wrapper makes it a bit clearer we're calling something within the context of the job classloader.

          One of the drawbacks of the wrapper is that it's harder to deal with checked exceptions. For example, MRAppMaster.isRecoverySupported used to propagate any IOException thrown by the user-provided committer code, but the wrapper now transforms this into a runtime exception. Not sure it really makes a whole lot of difference in practice for that case, but it is a subtle change in behavior. Do you think it's worth adding a wrapper form that can throw Exception (or maybe just IOException)? Thinking along similar lines as PrivilegedAction vs. PrivilegedExceptionAction in the doAs scenarios. I guess callers can catch YarnRuntimeException with the current patch and check the cause if they want/need to convert it back to a checked exception themselves, but curious on your thoughts.

          Otherwise I think the patch looks good. Couple of minor additional items:

          • Is there a reason the init transition calls MRAppMaster.createJobClassloader rather than having MRAppMaster.serviceInit do it? Seems more natural to have the serviceInit method initialize the MRAppMaster data members if possible.
          • Not strictly related to this JIRA, but I happened to notice that MRAppMaster.isRecoverySupported takes a committer argument that it totally ignores. Since we're in the area, it would be nice to clean that up by either removing the argument or use it rather than the committer member. Totally OK with me if you'd rather postpone that minor cleanup to another JIRA.
          Show
          Jason Lowe added a comment - Thanks for updating the patch. I do think the wrapper makes it a bit clearer we're calling something within the context of the job classloader. One of the drawbacks of the wrapper is that it's harder to deal with checked exceptions. For example, MRAppMaster.isRecoverySupported used to propagate any IOException thrown by the user-provided committer code, but the wrapper now transforms this into a runtime exception. Not sure it really makes a whole lot of difference in practice for that case, but it is a subtle change in behavior. Do you think it's worth adding a wrapper form that can throw Exception (or maybe just IOException)? Thinking along similar lines as PrivilegedAction vs. PrivilegedExceptionAction in the doAs scenarios. I guess callers can catch YarnRuntimeException with the current patch and check the cause if they want/need to convert it back to a checked exception themselves, but curious on your thoughts. Otherwise I think the patch looks good. Couple of minor additional items: Is there a reason the init transition calls MRAppMaster.createJobClassloader rather than having MRAppMaster.serviceInit do it? Seems more natural to have the serviceInit method initialize the MRAppMaster data members if possible. Not strictly related to this JIRA, but I happened to notice that MRAppMaster.isRecoverySupported takes a committer argument that it totally ignores. Since we're in the area, it would be nice to clean that up by either removing the argument or use it rather than the committer member. Totally OK with me if you'd rather postpone that minor cleanup to another JIRA.
          Hide
          Sangjin Lee added a comment -

          The thread context classloader is what makes this type of problems difficult to fix completely. The thread context classloader is problematic in so many ways. It has no formal contract defined, and it represents an ultimate backdoor for thwarting any modularity.

          Show
          Sangjin Lee added a comment - The thread context classloader is what makes this type of problems difficult to fix completely. The thread context classloader is problematic in so many ways. It has no formal contract defined, and it represents an ultimate backdoor for thwarting any modularity.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12656001/MAPREDUCE-5957.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. There were no new javadoc warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient:

          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesAttempts
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobs
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesTasks
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobConf
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServices

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

          Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4740//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4740//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/12656001/MAPREDUCE-5957.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 . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient: org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesAttempts org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobs org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesTasks org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobConf org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServices +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4740//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4740//console This message is automatically generated.
          Hide
          Sangjin Lee added a comment -

          Thanks for the feedback Jason.

          I do like the idea of providing a convenience method that wraps the logic of setting and unsetting the classloader around code. I think it will lead to easier and more obvious handling of this. I'll update the patch to do that (lambda would have been very nice btw).

          Also, a good catch about the committer event handler thread creation. I'll also handle that scenario.

          I think the latest patch should cover at least all these known cases, except for one. It's the composite service's serviceInit() and serviceStart(). In case of the speculator, it's added as a service to the parent composite service. As such, super.serviceInit() and super.serviceStart() will invoke its serviceInit() and serviceStart(). If it loads classes via configuration classloader or TCCL, it will break.

          Note that normal classloading will still work. For that matter, if any of these classes loads other classes in the normal way, that still works fine.

          Show
          Sangjin Lee added a comment - Thanks for the feedback Jason. I do like the idea of providing a convenience method that wraps the logic of setting and unsetting the classloader around code. I think it will lead to easier and more obvious handling of this. I'll update the patch to do that (lambda would have been very nice btw). Also, a good catch about the committer event handler thread creation. I'll also handle that scenario. I think the latest patch should cover at least all these known cases, except for one. It's the composite service's serviceInit() and serviceStart(). In case of the speculator, it's added as a service to the parent composite service. As such, super.serviceInit() and super.serviceStart() will invoke its serviceInit() and serviceStart(). If it loads classes via configuration classloader or TCCL, it will break. Note that normal classloading will still work. For that matter, if any of these classes loads other classes in the normal way, that still works fine.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12655890/MAPREDUCE-5957.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. There were no new javadoc warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient.

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

          Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4737//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4737//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/12655890/MAPREDUCE-5957.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 . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4737//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4737//console This message is automatically generated.
          Hide
          Jason Lowe added a comment -

          Yeah, seeing the second patch I can understand why you initially gravitated to the first approach. I agree that it is quite likely things will break as the code is maintained. Not only do we have to flip-flop each time a new custom class is loaded but also each time we invoke a method on a user-provided instance. So if a new committer method is added or what-not that needs to be called before we finally throw the classloader switch for good then that's another necessary flip-flop. That's annoying and probably won't be remembered when the new method invocation is added.

          If we do stick with the flip-flop approach then it'd be nice if we had a nice way to wrap such code with a common flip-flop wrapper. I'm thinking something akin to how UserGroupInformation.doAs works so we can wrap code with common logic and don't have to copy-n-paste the wrapper everywhere. However the wrapped code has to be marshaled in the form of a Runnable or what-not, so that might not be much better in the end.

          So I guess it comes down to weighing the likelihood this will ever be needed in practice or if simply setting the conf classloader will "just work." I'm not as worried about the reflection case since that's probably rare to do without already leveraging Hadoop's conf class property processing. However I'm worried about thread creation since I could see a case where a committer/speculator/whatever creates some threads in their constructor or some other method we invoke before throwing the final classloader switch. If those threads end up inheriting the system TCCL instead of the job classloader TCCL then we have a problem. If that indeed is what would happen then it comes down to how likely is it that user code will want to create threads in constructors/methods that are invoked before the final classloader switch. I don't know offhand, unfortunately.

          If we can convince ourselves the thread use by user methods invoked before the final loader switch is either a non-issue or super unlikely then I think we should go for the simpler fix to set the conf loader early. Otherwise I think we may be stuck doing the flip flop case just for correctness sake, and hopefully we can make that as painless and obvious as possible.

          Which reminds me, the CommitterEventHandler creates a thread with which it invokes methods on the output committer. That thread is going to have the system TCCL since it was created before the final classloader switch, correct? If so would we also have problems if the output committer does lazy thread creation or class loading when commit methods are invoked as the job runs? Seems like the CommitterEventHandler event handling thread needs the job classloader, if specified.

          Show
          Jason Lowe added a comment - Yeah, seeing the second patch I can understand why you initially gravitated to the first approach. I agree that it is quite likely things will break as the code is maintained. Not only do we have to flip-flop each time a new custom class is loaded but also each time we invoke a method on a user-provided instance. So if a new committer method is added or what-not that needs to be called before we finally throw the classloader switch for good then that's another necessary flip-flop. That's annoying and probably won't be remembered when the new method invocation is added. If we do stick with the flip-flop approach then it'd be nice if we had a nice way to wrap such code with a common flip-flop wrapper. I'm thinking something akin to how UserGroupInformation.doAs works so we can wrap code with common logic and don't have to copy-n-paste the wrapper everywhere. However the wrapped code has to be marshaled in the form of a Runnable or what-not, so that might not be much better in the end. So I guess it comes down to weighing the likelihood this will ever be needed in practice or if simply setting the conf classloader will "just work." I'm not as worried about the reflection case since that's probably rare to do without already leveraging Hadoop's conf class property processing. However I'm worried about thread creation since I could see a case where a committer/speculator/whatever creates some threads in their constructor or some other method we invoke before throwing the final classloader switch. If those threads end up inheriting the system TCCL instead of the job classloader TCCL then we have a problem. If that indeed is what would happen then it comes down to how likely is it that user code will want to create threads in constructors/methods that are invoked before the final classloader switch. I don't know offhand, unfortunately. If we can convince ourselves the thread use by user methods invoked before the final loader switch is either a non-issue or super unlikely then I think we should go for the simpler fix to set the conf loader early. Otherwise I think we may be stuck doing the flip flop case just for correctness sake, and hopefully we can make that as painless and obvious as possible. Which reminds me, the CommitterEventHandler creates a thread with which it invokes methods on the output committer. That thread is going to have the system TCCL since it was created before the final classloader switch, correct? If so would we also have problems if the output committer does lazy thread creation or class loading when commit methods are invoked as the job runs? Seems like the CommitterEventHandler event handling thread needs the job classloader, if specified.
          Hide
          Sangjin Lee added a comment -

          Uploaded a patch based on the alternate approach (setting and unsetting the job classloader on key sections of MRAppMaster code).

          The sections of code that need this treatment are any place that uses Configuration.getClass() to load a potentially non-hadoop class or any place that exercises methods on those objects that are instantiated this way.

          Show
          Sangjin Lee added a comment - Uploaded a patch based on the alternate approach (setting and unsetting the job classloader on key sections of MRAppMaster code). The sections of code that need this treatment are any place that uses Configuration.getClass() to load a potentially non-hadoop class or any place that exercises methods on those objects that are instantiated this way.
          Hide
          Sangjin Lee added a comment -

          Thanks for your comment Jason.

          I've been going back and forth between the two approaches on this. On the one hand, if we make the job classloader available early (but hold back setting the TCCL) this one time change would be sufficient to cover future changes when another instance of custom class loading is needed. That's why I gravitated to that approach. It is true that if the custom class needs to load another class via TCCL in its constructor (and a couple of other methods that get called by MRAppMaster) then it is a problem. It's a fairly uncommon scenario, but I can't say it should never happen.

          Surrounding custom class loading with setting and unsetting of the job classloader (both as the configuration classloader and as TCCL) does solve that problem. And I can't think of a case where making it available as TCCL during that time period would cause a different type of problems (along the line of MAPREDUCE-5751). Even if this thing invokes jetty initialization for example, it would have compelled its own copy of jetty and things would stay consistent within that class namespace.

          The main problem I have with this approach is that it's bit more expensive to maintain. Every time new code is added to load a custom class in MRAppMaster, we must remember to wrap it with setting and unsetting the job classloader. It's perfectly doable, but leaves room for making mistakes.

          I can bring up the second version of the patch that implements the other approach. Shall we discuss that a little more then? Let me know your thoughts.

          Show
          Sangjin Lee added a comment - Thanks for your comment Jason. I've been going back and forth between the two approaches on this. On the one hand, if we make the job classloader available early (but hold back setting the TCCL) this one time change would be sufficient to cover future changes when another instance of custom class loading is needed. That's why I gravitated to that approach. It is true that if the custom class needs to load another class via TCCL in its constructor (and a couple of other methods that get called by MRAppMaster) then it is a problem. It's a fairly uncommon scenario, but I can't say it should never happen. Surrounding custom class loading with setting and unsetting of the job classloader (both as the configuration classloader and as TCCL) does solve that problem. And I can't think of a case where making it available as TCCL during that time period would cause a different type of problems (along the line of MAPREDUCE-5751 ). Even if this thing invokes jetty initialization for example, it would have compelled its own copy of jetty and things would stay consistent within that class namespace. The main problem I have with this approach is that it's bit more expensive to maintain. Every time new code is added to load a custom class in MRAppMaster, we must remember to wrap it with setting and unsetting the job classloader. It's perfectly doable, but leaves room for making mistakes. I can bring up the second version of the patch that implements the other approach. Shall we discuss that a little more then? Let me know your thoughts.
          Hide
          Jason Lowe added a comment -

          Thanks Sangjin for the report and the patch. I'm pretty sure the test failures are unrelated as I've seen other recent JIRAs also complain about the same set of test failures.

          I think the proposed solution is reasonable and would probably work in most cases but will it cover all possible cases? In particular I'm thinking of a case where an output format/committer creates threads in its constructor or does reflection on its own with the current TCCL to load other classes. In those cases don't we need to have the TCCL set as well? I'm not a fan of flip-flopping the classloader around, but I'm wondering if we really should do that to cover whatever crazy stuff user code might try to do.

          Show
          Jason Lowe added a comment - Thanks Sangjin for the report and the patch. I'm pretty sure the test failures are unrelated as I've seen other recent JIRAs also complain about the same set of test failures. I think the proposed solution is reasonable and would probably work in most cases but will it cover all possible cases? In particular I'm thinking of a case where an output format/committer creates threads in its constructor or does reflection on its own with the current TCCL to load other classes. In those cases don't we need to have the TCCL set as well? I'm not a fan of flip-flopping the classloader around, but I'm wondering if we really should do that to cover whatever crazy stuff user code might try to do.
          Hide
          Sangjin Lee added a comment -

          I don't think the unit test failures are related to this patch. A jenkins run with a different patch have the same unit test failures (https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4733/).

          It's not clear yet whether these are related to jenkins (the port in question being held onto) or a more legitimate unit test issue.

          Show
          Sangjin Lee added a comment - I don't think the unit test failures are related to this patch. A jenkins run with a different patch have the same unit test failures ( https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4733/ ). It's not clear yet whether these are related to jenkins (the port in question being held onto) or a more legitimate unit test issue.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12655644/MAPREDUCE-5957.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. There were no new javadoc warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient:

          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesAttempts
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobs
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesTasks
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobConf
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServices

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

          Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4736//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4736//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/12655644/MAPREDUCE-5957.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 . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient: org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesAttempts org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobs org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesTasks org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobConf org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServices +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4736//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4736//console This message is automatically generated.
          Hide
          Sangjin Lee added a comment -

          Likely to be an existing issue? Kicking it off one more time.

          Show
          Sangjin Lee added a comment - Likely to be an existing issue? Kicking it off one more time.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12655644/MAPREDUCE-5957.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. There were no new javadoc warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient:

          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesAttempts
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobs
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesTasks
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobConf
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServices

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

          Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4735//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4735//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/12655644/MAPREDUCE-5957.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 . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient: org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesAttempts org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobs org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesTasks org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobConf org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServices +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4735//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4735//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12655644/MAPREDUCE-5957.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. There were no new javadoc warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient:

          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesAttempts
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobs
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesTasks
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobConf
          org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServices

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

          Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4731//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4731//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/12655644/MAPREDUCE-5957.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 . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient: org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesAttempts org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobs org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesTasks org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobConf org.apache.hadoop.mapreduce.v2.app.webapp.TestAMWebServices +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4731//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4731//console This message is automatically generated.
          Hide
          Sangjin Lee added a comment -

          Updated the patch with the unit test.

          Show
          Sangjin Lee added a comment - Updated the patch with the unit test.
          Hide
          Sangjin Lee added a comment -

          I am working on a unit test that can confirm the bug and the fix. I'll submit a new patch with the unit test soon.

          In the meantime, I would greatly appreciate your feedback on this. The main code changes are not affected by the unit test change.

          Show
          Sangjin Lee added a comment - I am working on a unit test that can confirm the bug and the fix. I'll submit a new patch with the unit test soon. In the meantime, I would greatly appreciate your feedback on this. The main code changes are not affected by the unit test change.
          Hide
          Sangjin Lee added a comment -

          Ping?

          Show
          Sangjin Lee added a comment - Ping?
          Hide
          Sangjin Lee added a comment -

          Tested with a pseudo-distributed cluster by specifying a custom output format class as well as a custom speculator class. Confirmed that ClassNotFoundException is thrown without the fix and verified the issue goes away with the fix.

          Show
          Sangjin Lee added a comment - Tested with a pseudo-distributed cluster by specifying a custom output format class as well as a custom speculator class. Confirmed that ClassNotFoundException is thrown without the fix and verified the issue goes away with the fix.
          Hide
          Hadoop QA added a comment -

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

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

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

          +1 javadoc. There were no new javadoc 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common.

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

          Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4704//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4704//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/12653711/MAPREDUCE-5957.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4704//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/4704//console This message is automatically generated.
          Hide
          Sangjin Lee added a comment -

          Patch that implements approach (1)

          Show
          Sangjin Lee added a comment - Patch that implements approach (1)
          Hide
          Sangjin Lee added a comment -

          The gist of this issue is regarding the use of Configuration.getClass() and the use of the thread context classloader (TCCL). Currently MRApps.setJobClassLoader() sets both the configuration classloader and the TCCL at the same time. So once setJobClassLoader() is called, it is made available in both contexts.

          MAPREDUCE-5751 was caused because the job classloader was made available too early as the TCCL. This issue is caused because the job classloader is made available too late as the configuration classloader.

          The normal classloading scheme (one class initializing another class via normal use or even Class.forName) is unaffected by this if my understanding is correct.

          I see two possible approaches for this:
          (1) separate the timing of setting the job classloader as the configuration classloader and the TCCL
          I think while setting the TCCL should be delayed as much as possible (i.e. the current timing), the job classloader can be installed as the configuration classloader much earlier. If the configuration loads a user class, that's precisely what we need. If it loads a system class, the job classloader will delegate anyhow. I don't think there is harm in setting the configuration classloader early.

          (2) set and unset the job classloader around the code that loads classes from the configuration
          Identify the code points in MRAppMaster where Configuration.getClass() is needed, and set and unset the job classloader around them. Although this would also solve this problem, the downside is that one needs to make a determination that the job classloader is needed and set/unset it. This is potentially brittle.

          I think (1) is a more robust solution to this problem. Do you see an issue with taking that approach?

          I don't think the task (YarnChild) is affected by this.

          Show
          Sangjin Lee added a comment - The gist of this issue is regarding the use of Configuration.getClass() and the use of the thread context classloader (TCCL). Currently MRApps.setJobClassLoader() sets both the configuration classloader and the TCCL at the same time. So once setJobClassLoader() is called, it is made available in both contexts. MAPREDUCE-5751 was caused because the job classloader was made available too early as the TCCL . This issue is caused because the job classloader is made available too late as the configuration classloader . The normal classloading scheme (one class initializing another class via normal use or even Class.forName) is unaffected by this if my understanding is correct. I see two possible approaches for this: (1) separate the timing of setting the job classloader as the configuration classloader and the TCCL I think while setting the TCCL should be delayed as much as possible (i.e. the current timing), the job classloader can be installed as the configuration classloader much earlier. If the configuration loads a user class, that's precisely what we need. If it loads a system class, the job classloader will delegate anyhow. I don't think there is harm in setting the configuration classloader early. (2) set and unset the job classloader around the code that loads classes from the configuration Identify the code points in MRAppMaster where Configuration.getClass() is needed, and set and unset the job classloader around them. Although this would also solve this problem, the downside is that one needs to make a determination that the job classloader is needed and set/unset it. This is potentially brittle. I think (1) is a more robust solution to this problem. Do you see an issue with taking that approach? I don't think the task (YarnChild) is affected by this.
          Hide
          Sangjin Lee added a comment -

          I'm thinking of setting and unsetting the job classloader when MRAppMaster needs to load a class from the configuration. I'll submit a patch for this soon.

          Show
          Sangjin Lee added a comment - I'm thinking of setting and unsetting the job classloader when MRAppMaster needs to load a class from the configuration. I'll submit a patch for this soon.
          Hide
          Sangjin Lee added a comment -

          This is a regression introduced by MAPREDUCE-5751.

          MAPREDUCE-5751 moved setting of the job classloader from before the MRAppMaster init to the end of the MRAppMaster start. However, some classes are compelled from the configuration in between these two points, such as the output format class, the speculator class, etc. The job classloader must be used to load those classes or they will not be found.

          Show
          Sangjin Lee added a comment - This is a regression introduced by MAPREDUCE-5751 . MAPREDUCE-5751 moved setting of the job classloader from before the MRAppMaster init to the end of the MRAppMaster start. However, some classes are compelled from the configuration in between these two points, such as the output format class, the speculator class, etc. The job classloader must be used to load those classes or they will not be found.

            People

            • Assignee:
              Sangjin Lee
              Reporter:
              Sangjin Lee
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development