Uploaded image for project: 'Hadoop YARN'
  1. Hadoop YARN
  2. YARN-1931

Private API change in YARN-1824 in 2.4 broke compatibility with previous releases

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4.0
    • Fix Version/s: 2.4.1
    • Component/s: applications
    • Labels:
      None
    • Target Version/s:

      Description

      YARN-1824 broke compatibility with previous 2.x releases by changes the API's in org.apache.hadoop.yarn.util.Apps.

      {setEnvFromInputString,addToEnvironment}

      The old api should be added back in.

      This affects any ApplicationMasters who were using this api. It also breaks previously built MapReduce libraries from working with the new Yarn release as MR uses this api.

      1. YARN-1931.patch
        0.9 kB
        Sandy Ryza
      2. YARN-1931-1.patch
        1 kB
        Sandy Ryza
      3. YARN-1931-2.patch
        2 kB
        Sandy Ryza

        Issue Links

          Activity

          Hide
          tgraves Thomas Graves added a comment -

          I'm also a bit confused by this api marking. Its marked as private but it claims to be utilities for applications.
          This api was not marked as private in 0.23, which is when Spark started using the api.

          If its truly private to yarn then MR shouldn't be using the api.

          Show
          tgraves Thomas Graves added a comment - I'm also a bit confused by this api marking. Its marked as private but it claims to be utilities for applications. This api was not marked as private in 0.23, which is when Spark started using the api. If its truly private to yarn then MR shouldn't be using the api.
          Hide
          zjshen Zhijie Shen added a comment -

          Right, we saw the same problem recently.

          I'm also a bit confused by this api marking. Its marked as private but it claims to be utilities for applications.

          It seems to be better to mark Apps as @Public. MR, as an application framework, has used it. Spark is using it, and I think there should be other applications as well.

          Show
          zjshen Zhijie Shen added a comment - Right, we saw the same problem recently. I'm also a bit confused by this api marking. Its marked as private but it claims to be utilities for applications. It seems to be better to mark Apps as @Public. MR, as an application framework, has used it. Spark is using it, and I think there should be other applications as well.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          This API got marked Private as part of the YARN API stabilization effort (YARN-386).

          MapReduce still uses a lot of YARN private APIs. Unfortunately that cannot be the benchmark for other applications. App developers should really just consult the javadocs where this class will be completely absent.

          We should think twice before making it Public. This was at best a clumsily written class with utilities added on demand.

          Overall, I think Spark should change. 0.23 YARN APIs are NOT compatible with 2.2 YARN APIs after YARN-386.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - This API got marked Private as part of the YARN API stabilization effort ( YARN-386 ). MapReduce still uses a lot of YARN private APIs. Unfortunately that cannot be the benchmark for other applications. App developers should really just consult the javadocs where this class will be completely absent. We should think twice before making it Public. This was at best a clumsily written class with utilities added on demand. Overall, I think Spark should change. 0.23 YARN APIs are NOT compatible with 2.2 YARN APIs after YARN-386 .
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          One more comment regarding the change itself - it was done so as to force the callers to explicitly decide on the CLASSPATH separator to support cases like YARN-1824.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - One more comment regarding the change itself - it was done so as to force the callers to explicitly decide on the CLASSPATH separator to support cases like YARN-1824 .
          Hide
          sandyr Sandy Ryza added a comment -

          +1 to adding the old API back in. The old API isn't the right one going forward, and we should deprecate it, but keeping it will save a lot of trouble for those who want to run an older version of Spark or MR on a recent version of YARN.

          Show
          sandyr Sandy Ryza added a comment - +1 to adding the old API back in. The old API isn't the right one going forward, and we should deprecate it, but keeping it will save a lot of trouble for those who want to run an older version of Spark or MR on a recent version of YARN.
          Hide
          tgraves Thomas Graves added a comment -

          We are going to change Spark since we have no choice if we want to be compatible with both 2.4 and 2.3.

          I'm fine with not making it public, but then MR should be changed to not rely on it so this breakage doesn't happen in the future. And I still think the old api should be put back in for backwards compatibility.

          Show
          tgraves Thomas Graves added a comment - We are going to change Spark since we have no choice if we want to be compatible with both 2.4 and 2.3. I'm fine with not making it public, but then MR should be changed to not rely on it so this breakage doesn't happen in the future. And I still think the old api should be put back in for backwards compatibility.
          Hide
          sandyr Sandy Ryza added a comment -

          Attached a patch that adds back the old method as deprecated.

          Show
          sandyr Sandy Ryza added a comment - Attached a patch that adds back the old method as deprecated.
          Hide
          tgraves Thomas Graves added a comment -

          thanks Sandy, setEnvFromInputString also changed.

          Show
          tgraves Thomas Graves added a comment - thanks Sandy, setEnvFromInputString also changed.
          Hide
          sandyr Sandy Ryza added a comment -

          Oops, updated patch includes setEnvFromInputString as well.

          Show
          sandyr Sandy Ryza added a comment - Oops, updated patch includes setEnvFromInputString as well.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          I'm okay with this for now. But I'm requesting YARN app frameworks like TEZ, Spark and Twill to do a scan of their projects and get rid of YARN private APIs that are in use.

          Assigning to Sandy. Reducing priority and fixing title.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - I'm okay with this for now. But I'm requesting YARN app frameworks like TEZ, Spark and Twill to do a scan of their projects and get rid of YARN private APIs that are in use. Assigning to Sandy. Reducing priority and fixing title.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          Please leave comments on the deprecated APIs as to why they are deprecated and what should be used instead.

          It'll be great to also put a class level javadoc comment that this is indeed private and that folks should not use it directly.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - Please leave comments on the deprecated APIs as to why they are deprecated and what should be used instead. It'll be great to also put a class level javadoc comment that this is indeed private and that folks should not use it directly.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12639840/YARN-1931.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-yarn-project/hadoop-yarn/hadoop-yarn-common.

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

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

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12639840/YARN-1931.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-yarn-project/hadoop-yarn/hadoop-yarn-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/3560//testReport/ Console output: https://builds.apache.org/job/PreCommit-YARN-Build/3560//console This message is automatically generated.
          Hide
          tgraves Thomas Graves added a comment - - edited

          why the change in priority?

          Show
          tgraves Thomas Graves added a comment - - edited why the change in priority?
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          The priority change is because we all agreed this is a private API and that the break in compatibility in private APIs is not unexpected.

          We cannot arbitrarily support private APIs for indeterminate amounts of time in YARN, but we are making a compromise here for the short term given multiple projects are affected due to the private API change.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - The priority change is because we all agreed this is a private API and that the break in compatibility in private APIs is not unexpected. We cannot arbitrarily support private APIs for indeterminate amounts of time in YARN, but we are making a compromise here for the short term given multiple projects are affected due to the private API change.
          Hide
          tgraves Thomas Graves added a comment -

          Sandy can you please add a comment like Vinod requested about why they are there and what should be used.

          I filed a Spark jira to go through the YARN api there and change if using any private ones.

          Show
          tgraves Thomas Graves added a comment - Sandy can you please add a comment like Vinod requested about why they are there and what should be used. I filed a Spark jira to go through the YARN api there and change if using any private ones.
          Hide
          sandyr Sandy Ryza added a comment -

          It'll be great to also put a class level javadoc comment that this is indeed private and that folks should not use it directly.

          addToEnvironment environment and setEnvFromInputString are both used by MapReduce. As there is no up-to-date developer documentation, the MapReduce code is currently the best place to learn about how to build ContainerLaunchContexts, so I think it would be good to make sure it showcases the best practices for other YARN applications. Also, addToEnvironment is marked Public. Would it make sense to mark the class Public and mark all methods other than these two as Private?

          Show
          sandyr Sandy Ryza added a comment - It'll be great to also put a class level javadoc comment that this is indeed private and that folks should not use it directly. addToEnvironment environment and setEnvFromInputString are both used by MapReduce. As there is no up-to-date developer documentation, the MapReduce code is currently the best place to learn about how to build ContainerLaunchContexts, so I think it would be good to make sure it showcases the best practices for other YARN applications. Also, addToEnvironment is marked Public. Would it make sense to mark the class Public and mark all methods other than these two as Private?
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          Please don't do this. Two wrongs make a right. These are private APIs. Just like Records any many other things that MR is still using directly. We should fix MR to not depend on those, not make methods public which are never intended to be public.

          Also if MR code is the best place to learn how to write apps, this community has got it all wrong. How about helping with some documentation?

          The class itself is marked private. So it won't appear in javadoc. It will greatly help if developers of ecosystem projects refer to published javadocs instead of relying on source code.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - Please don't do this. Two wrongs make a right. These are private APIs. Just like Records any many other things that MR is still using directly. We should fix MR to not depend on those, not make methods public which are never intended to be public. Also if MR code is the best place to learn how to write apps, this community has got it all wrong. How about helping with some documentation? The class itself is marked private. So it won't appear in javadoc. It will greatly help if developers of ecosystem projects refer to published javadocs instead of relying on source code.
          Hide
          sandyr Sandy Ryza added a comment -

          Please don't do this. Two wrongs make a right. These are private APIs. Just like Records any many other things that MR is still using directly. We should fix MR to not depend on those, not make methods public which are never intended to be public.

          If MR shouldn't be using these APIs, I'm happy work on moving it off of them. However, these APIs provide functionality that nearly all frameworks built on YARN need. YARN doesn't use them internally. Why not expose them publicly?

          The class itself is marked private. So it won't appear in javadoc.

          Right, this is why I'm suggesting marking it Public (of course only if we determine that it's reasonable for apps to take advantage of it).

          Show
          sandyr Sandy Ryza added a comment - Please don't do this. Two wrongs make a right. These are private APIs. Just like Records any many other things that MR is still using directly. We should fix MR to not depend on those, not make methods public which are never intended to be public. If MR shouldn't be using these APIs, I'm happy work on moving it off of them. However, these APIs provide functionality that nearly all frameworks built on YARN need. YARN doesn't use them internally. Why not expose them publicly? The class itself is marked private. So it won't appear in javadoc. Right, this is why I'm suggesting marking it Public (of course only if we determine that it's reasonable for apps to take advantage of it).
          Hide
          sandyr Sandy Ryza added a comment -

          To be clear, when I say "these APIs", I don't mean all the Private APIs that MR is using. Just the two being discussed on this JIRA.

          Show
          sandyr Sandy Ryza added a comment - To be clear, when I say "these APIs", I don't mean all the Private APIs that MR is using. Just the two being discussed on this JIRA.
          Hide
          jianhe Jian He added a comment -

          This was at best a clumsily written class with utilities added on demand.

          We can add the methods back, but I also prefer that not marking it as Public. If needed, we should at least craft a newly well-written util class also with a better naming like AppUtils instead of Apps which is unclear for other new people working on YARN.

          Show
          jianhe Jian He added a comment - This was at best a clumsily written class with utilities added on demand. We can add the methods back, but I also prefer that not marking it as Public. If needed, we should at least craft a newly well-written util class also with a better naming like AppUtils instead of Apps which is unclear for other new people working on YARN.
          Hide
          tgraves Thomas Graves added a comment -

          I also agree it makes more sense to make a new utility class. Put these back for backwards compatibility but ask people to move to the new api.

          Should this class be marked LimitedPrivate(

          {MapReduce, Yarn}

          )? Its also missing the interface stability annotation.

          Show
          tgraves Thomas Graves added a comment - I also agree it makes more sense to make a new utility class. Put these back for backwards compatibility but ask people to move to the new api. Should this class be marked LimitedPrivate( {MapReduce, Yarn} )? Its also missing the interface stability annotation.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          We can add the methods back, but I also prefer that not marking it as Public.

          +1. Let's just put in the method here, add javadoc like I requested above and do everything else in other JIRAs.

          If needed, we should at least craft a newly well-written util class also with a better naming like AppUtils instead of Apps which is unclear for other new people working on YARN.

          +1. And may be a better name than Apps or AppUtils But definitely another JIRA.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - We can add the methods back, but I also prefer that not marking it as Public. +1. Let's just put in the method here, add javadoc like I requested above and do everything else in other JIRAs. If needed, we should at least craft a newly well-written util class also with a better naming like AppUtils instead of Apps which is unclear for other new people working on YARN. +1. And may be a better name than Apps or AppUtils But definitely another JIRA.
          Hide
          sandyr Sandy Ryza added a comment -

          Filed YARN-1948 for providing public versions of the methods.

          Show
          sandyr Sandy Ryza added a comment - Filed YARN-1948 for providing public versions of the methods.
          Hide
          sandyr Sandy Ryza added a comment -

          Updated patch adds comments to clarify what's going on.

          Show
          sandyr Sandy Ryza added a comment - Updated patch adds comments to clarify what's going on.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12640367/YARN-1931-2.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-yarn-project/hadoop-yarn/hadoop-yarn-common.

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

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

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12640367/YARN-1931-2.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-yarn-project/hadoop-yarn/hadoop-yarn-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/3573//testReport/ Console output: https://builds.apache.org/job/PreCommit-YARN-Build/3573//console This message is automatically generated.
          Hide
          tgraves Thomas Graves added a comment -

          +1, Thanks Sandy. I'll wait til this afternoon to commit in case Vinod has any further comments.

          Show
          tgraves Thomas Graves added a comment - +1, Thanks Sandy. I'll wait til this afternoon to commit in case Vinod has any further comments.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #5531 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5531/)
          YARN-1931. Private API change in YARN-1824 in 2.4 broke compatibility with previous releases (Sandy Ryza via tgraves) (tgraves: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1588281)

          • /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
          • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/Apps.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #5531 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5531/ ) YARN-1931 . Private API change in YARN-1824 in 2.4 broke compatibility with previous releases (Sandy Ryza via tgraves) (tgraves: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1588281 ) /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/Apps.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #544 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/544/)
          YARN-1931. Private API change in YARN-1824 in 2.4 broke compatibility with previous releases (Sandy Ryza via tgraves) (tgraves: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1588281)

          • /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
          • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/Apps.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #544 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/544/ ) YARN-1931 . Private API change in YARN-1824 in 2.4 broke compatibility with previous releases (Sandy Ryza via tgraves) (tgraves: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1588281 ) /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/Apps.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #1736 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1736/)
          YARN-1931. Private API change in YARN-1824 in 2.4 broke compatibility with previous releases (Sandy Ryza via tgraves) (tgraves: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1588281)

          • /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
          • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/Apps.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #1736 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1736/ ) YARN-1931 . Private API change in YARN-1824 in 2.4 broke compatibility with previous releases (Sandy Ryza via tgraves) (tgraves: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1588281 ) /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/Apps.java
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1761 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1761/)
          YARN-1931. Private API change in YARN-1824 in 2.4 broke compatibility with previous releases (Sandy Ryza via tgraves) (tgraves: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1588281)

          • /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
          • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/Apps.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1761 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1761/ ) YARN-1931 . Private API change in YARN-1824 in 2.4 broke compatibility with previous releases (Sandy Ryza via tgraves) (tgraves: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1588281 ) /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/Apps.java

            People

            • Assignee:
              sandyr Sandy Ryza
              Reporter:
              tgraves Thomas Graves
            • Votes:
              0 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development