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

Fix up incompatible changes introduced on ContainerStatus and NodeReport

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.8.0, 2.9.0
    • Fix Version/s: 2.8.0, 3.0.0-alpha2
    • Component/s: api
    • Labels:
      None

      Description

      YARN-2882 and YARN-5430 broke compatibility by adding abstract methods to ContainerStatus. Since ContainerStatus is a Public-Stable class, adding abstract methods to this class breaks any extensions.

      To fix this, we should add default implementations to these new methods and not leave them as abstract.

      1. YARN-5184-branch-2.8.01.patch
        3 kB
        Sangjin Lee
      2. YARN-5184.01.patch
        4 kB
        Sangjin Lee

        Issue Links

          Activity

          Hide
          andrew.wang Andrew Wang added a comment -

          Looks like this is something we should address in 3.0.0-alpha2.

          Show
          andrew.wang Andrew Wang added a comment - Looks like this is something we should address in 3.0.0-alpha2.
          Hide
          sjlee0 Sangjin Lee added a comment -

          Since 3.0.0 permits backward incompatible changes, this should not be a blocker for 3.0.0-alpha2. If 2.9.0 is ever released, this should be fixed before that.

          Show
          sjlee0 Sangjin Lee added a comment - Since 3.0.0 permits backward incompatible changes, this should not be a blocker for 3.0.0-alpha2. If 2.9.0 is ever released, this should be fixed before that.
          Hide
          andrew.wang Andrew Wang added a comment -

          Karthik, could you summarize the incompatibilities from these two JIRAs, and the potential fix? If the changes were made intentionally, then I agree with Sangjin and we can leave them for 3.0.0-alpha2. However, if they were accidental or can be easily fixed to make life easier for downstreams, then I would like to address this in alpha2.

          Show
          andrew.wang Andrew Wang added a comment - Karthik, could you summarize the incompatibilities from these two JIRAs, and the potential fix? If the changes were made intentionally, then I agree with Sangjin and we can leave them for 3.0.0-alpha2. However, if they were accidental or can be easily fixed to make life easier for downstreams, then I would like to address this in alpha2.
          Hide
          sjlee0 Sangjin Lee added a comment -

          The changes are the introduction of abstract getter/setter methods.

          Show
          sjlee0 Sangjin Lee added a comment - The changes are the introduction of abstract getter/setter methods.
          Hide
          sjlee0 Sangjin Lee added a comment -

          Adding a proposed patch for branch-2 (for 2.9.0) for feedback. I'll also add a branch-2.8 patch (for 2.8.0) which is a subset of the 2.9.0 changes.

          I don't have a strong opinion on what should be done on the trunk (3.0.0). I think it is OK to leave it as is, because this change's sole purpose is to maintain API backward compatibility. One could regard this as 2.8.0/2.9.0 providing this band-aid and 3.0.0 taking the band-aid off.

          Show
          sjlee0 Sangjin Lee added a comment - Adding a proposed patch for branch-2 (for 2.9.0) for feedback. I'll also add a branch-2.8 patch (for 2.8.0) which is a subset of the 2.9.0 changes. I don't have a strong opinion on what should be done on the trunk (3.0.0). I think it is OK to leave it as is, because this change's sole purpose is to maintain API backward compatibility. One could regard this as 2.8.0/2.9.0 providing this band-aid and 3.0.0 taking the band-aid off.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 21s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s 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 mvninstall 6m 57s branch-2 passed
          +1 compile 0m 23s branch-2 passed with JDK v1.8.0_101
          +1 compile 0m 26s branch-2 passed with JDK v1.7.0_111
          +1 checkstyle 0m 15s branch-2 passed
          +1 mvnsite 0m 30s branch-2 passed
          +1 mvneclipse 0m 13s branch-2 passed
          +1 findbugs 1m 17s branch-2 passed
          +1 javadoc 0m 17s branch-2 passed with JDK v1.8.0_101
          +1 javadoc 0m 20s branch-2 passed with JDK v1.7.0_111
          +1 mvninstall 0m 25s the patch passed
          +1 compile 0m 23s the patch passed with JDK v1.8.0_101
          +1 javac 0m 23s the patch passed
          +1 compile 0m 23s the patch passed with JDK v1.7.0_111
          +1 javac 0m 23s the patch passed
          +1 checkstyle 0m 11s the patch passed
          +1 mvnsite 0m 29s the patch passed
          +1 mvneclipse 0m 11s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 30s the patch passed
          +1 javadoc 0m 19s the patch passed with JDK v1.8.0_101
          +1 javadoc 0m 17s the patch passed with JDK v1.7.0_111
          +1 unit 0m 28s hadoop-yarn-api in the patch passed with JDK v1.7.0_111.
          +1 asflicense 0m 19s The patch does not generate ASF License warnings.
          18m 8s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:b59b8b7
          JIRA Issue YARN-5184
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835834/YARN-5184-branch-2.poc.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 6387e5af8887 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision branch-2 / 0f224d4
          Default Java 1.7.0_111
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_101 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111
          findbugs v3.0.0
          JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13637/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13637/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 21s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s 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 mvninstall 6m 57s branch-2 passed +1 compile 0m 23s branch-2 passed with JDK v1.8.0_101 +1 compile 0m 26s branch-2 passed with JDK v1.7.0_111 +1 checkstyle 0m 15s branch-2 passed +1 mvnsite 0m 30s branch-2 passed +1 mvneclipse 0m 13s branch-2 passed +1 findbugs 1m 17s branch-2 passed +1 javadoc 0m 17s branch-2 passed with JDK v1.8.0_101 +1 javadoc 0m 20s branch-2 passed with JDK v1.7.0_111 +1 mvninstall 0m 25s the patch passed +1 compile 0m 23s the patch passed with JDK v1.8.0_101 +1 javac 0m 23s the patch passed +1 compile 0m 23s the patch passed with JDK v1.7.0_111 +1 javac 0m 23s the patch passed +1 checkstyle 0m 11s the patch passed +1 mvnsite 0m 29s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 30s the patch passed +1 javadoc 0m 19s the patch passed with JDK v1.8.0_101 +1 javadoc 0m 17s the patch passed with JDK v1.7.0_111 +1 unit 0m 28s hadoop-yarn-api in the patch passed with JDK v1.7.0_111. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 18m 8s Subsystem Report/Notes Docker Image:yetus/hadoop:b59b8b7 JIRA Issue YARN-5184 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835834/YARN-5184-branch-2.poc.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 6387e5af8887 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision branch-2 / 0f224d4 Default Java 1.7.0_111 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_101 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111 findbugs v3.0.0 JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13637/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api Console output https://builds.apache.org/job/PreCommit-YARN-Build/13637/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s 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 mvninstall 10m 8s branch-2.8 passed
          +1 compile 0m 23s branch-2.8 passed with JDK v1.8.0_101
          +1 compile 0m 23s branch-2.8 passed with JDK v1.7.0_111
          +1 checkstyle 0m 15s branch-2.8 passed
          +1 mvnsite 0m 30s branch-2.8 passed
          +1 mvneclipse 0m 14s branch-2.8 passed
          +1 findbugs 1m 13s branch-2.8 passed
          +1 javadoc 0m 18s branch-2.8 passed with JDK v1.8.0_101
          +1 javadoc 0m 18s branch-2.8 passed with JDK v1.7.0_111
          +1 mvninstall 0m 23s the patch passed
          +1 compile 0m 20s the patch passed with JDK v1.8.0_101
          +1 javac 0m 20s the patch passed
          +1 compile 0m 21s the patch passed with JDK v1.7.0_111
          +1 javac 0m 21s the patch passed
          +1 checkstyle 0m 11s the patch passed
          +1 mvnsite 0m 25s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 22s the patch passed
          +1 javadoc 0m 13s the patch passed with JDK v1.8.0_101
          +1 javadoc 0m 17s the patch passed with JDK v1.7.0_111
          +1 unit 0m 24s hadoop-yarn-api in the patch passed with JDK v1.7.0_111.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          20m 29s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:5af2af1
          JIRA Issue YARN-5184
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835838/YARN-5184-branch-2.8.poc.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 59ee0c991559 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision branch-2.8 / 83bb428
          Default Java 1.7.0_111
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_101 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111
          findbugs v3.0.0
          JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13638/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13638/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 14s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s 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 mvninstall 10m 8s branch-2.8 passed +1 compile 0m 23s branch-2.8 passed with JDK v1.8.0_101 +1 compile 0m 23s branch-2.8 passed with JDK v1.7.0_111 +1 checkstyle 0m 15s branch-2.8 passed +1 mvnsite 0m 30s branch-2.8 passed +1 mvneclipse 0m 14s branch-2.8 passed +1 findbugs 1m 13s branch-2.8 passed +1 javadoc 0m 18s branch-2.8 passed with JDK v1.8.0_101 +1 javadoc 0m 18s branch-2.8 passed with JDK v1.7.0_111 +1 mvninstall 0m 23s the patch passed +1 compile 0m 20s the patch passed with JDK v1.8.0_101 +1 javac 0m 20s the patch passed +1 compile 0m 21s the patch passed with JDK v1.7.0_111 +1 javac 0m 21s the patch passed +1 checkstyle 0m 11s the patch passed +1 mvnsite 0m 25s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 22s the patch passed +1 javadoc 0m 13s the patch passed with JDK v1.8.0_101 +1 javadoc 0m 17s the patch passed with JDK v1.7.0_111 +1 unit 0m 24s hadoop-yarn-api in the patch passed with JDK v1.7.0_111. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 20m 29s Subsystem Report/Notes Docker Image:yetus/hadoop:5af2af1 JIRA Issue YARN-5184 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835838/YARN-5184-branch-2.8.poc.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 59ee0c991559 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision branch-2.8 / 83bb428 Default Java 1.7.0_111 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_101 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111 findbugs v3.0.0 JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13638/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api Console output https://builds.apache.org/job/PreCommit-YARN-Build/13638/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          kasha Karthik Kambatla added a comment -

          I would think this falls under the purview of API compatibility. To make incompatible changes, we need to deprecate it for a major release first. In that sense, we should do a fix for trunk as well. I am open to adding a new notion of API compatibility for class extensions, and call out potential incompatibilities on major releases. I am also open to adding a new annotation called Extendable if we think that is helpful.

          In the patch, the methods throw UnsupportedException. I had suggested this in an offline discussion with Sangjin, but I am not sure about it now. With this update, the downstream classes would continue to compile. This, however, can lead to RuntimeExceptions which is likely worse than compile time issues. I guess it depends on whether these methods end up being called. I can't think of a good solution, but thought I should bring this up.

          Show
          kasha Karthik Kambatla added a comment - I would think this falls under the purview of API compatibility. To make incompatible changes, we need to deprecate it for a major release first. In that sense, we should do a fix for trunk as well. I am open to adding a new notion of API compatibility for class extensions, and call out potential incompatibilities on major releases. I am also open to adding a new annotation called Extendable if we think that is helpful. In the patch, the methods throw UnsupportedException. I had suggested this in an offline discussion with Sangjin, but I am not sure about it now. With this update, the downstream classes would continue to compile. This, however, can lead to RuntimeExceptions which is likely worse than compile time issues. I guess it depends on whether these methods end up being called. I can't think of a good solution, but thought I should bring this up.
          Hide
          sjlee0 Sangjin Lee added a comment -

          In the patch, the methods throw UnsupportedException. I had suggested this in an offline discussion with Sangjin, but I am not sure about it now. With this update, the downstream classes would continue to compile. This, however, can lead to RuntimeExceptions which is likely worse than compile time issues. I guess it depends on whether these methods end up being called. I can't think of a good solution, but thought I should bring this up.

          I thought about that, and I think the risk is pretty minimal. No subclass/user of this class that exists today would be affected because they do not invoke the new methods as obviously they don't know the new methods. The only downside is later if a new subclass is created and forgets to override these methods (which will not be caught by the compiler). But then I suppose these classes are not really to be subclassed by users of Hadoop other than test or mock cases.

          Another way to solve it would be to provide real implementations that would work for all cases. But I don't think that's possible as the real implementations come from PBImpl classes...

          Show
          sjlee0 Sangjin Lee added a comment - In the patch, the methods throw UnsupportedException. I had suggested this in an offline discussion with Sangjin, but I am not sure about it now. With this update, the downstream classes would continue to compile. This, however, can lead to RuntimeExceptions which is likely worse than compile time issues. I guess it depends on whether these methods end up being called. I can't think of a good solution, but thought I should bring this up. I thought about that, and I think the risk is pretty minimal. No subclass/user of this class that exists today would be affected because they do not invoke the new methods as obviously they don't know the new methods. The only downside is later if a new subclass is created and forgets to override these methods (which will not be caught by the compiler). But then I suppose these classes are not really to be subclassed by users of Hadoop other than test or mock cases. Another way to solve it would be to provide real implementations that would work for all cases. But I don't think that's possible as the real implementations come from PBImpl classes...
          Hide
          kasha Karthik Kambatla added a comment -

          What if the methods in question are used by rest of YARN code? If someone subclasses this to provide an alternate implementation, calling these methods would lead to RTE.

          I agree the current implementation is likely the best we can do. The reason I am raising this concern is to see if there is a need for one more annotation that clarifies our intent about users extending classes to be plugged in.

          Show
          kasha Karthik Kambatla added a comment - What if the methods in question are used by rest of YARN code? If someone subclasses this to provide an alternate implementation, calling these methods would lead to RTE. I agree the current implementation is likely the best we can do. The reason I am raising this concern is to see if there is a need for one more annotation that clarifies our intent about users extending classes to be plugged in.
          Hide
          kasha Karthik Kambatla added a comment -

          Assigning to Sangjin as he is working on patch.

          Show
          kasha Karthik Kambatla added a comment - Assigning to Sangjin as he is working on patch.
          Hide
          sjlee0 Sangjin Lee added a comment -

          What if the methods in question are used by rest of YARN code? If someone subclasses this to provide an alternate implementation, calling these methods would lead to RTE.

          Yes, I agree that it is a very valid concern, as a runtime exception is a poor substitute for a compile-time error. I think a mitigating circumstance here is that these classes are normally extended only by the canonical implementation (protobuf impl): ContainerStatusPBImpl for ContainerStatus, and NodeReportPBImpl for NodeReport. Thus, the real likelihood that these classes would be extended by other classes would be pretty small, but it cannot be ruled out. In practice, the only times these are extended might be test classes (mock classes).

          In that sense, actually it could be an argument for not moving this change to the trunk. By doing so, we would be extending this window of potential trouble into 3.0. Since 2.8 has not been released, we could consider this as adding compatible default implementation methods which will then be made abstract in 3.0. Thoughts?

          The reason I am raising this concern is to see if there is a need for one more annotation that clarifies our intent about users extending classes to be plugged in.

          I do agree that we probably need another audience annotation for whether users may extend/implement classes (which I also proposed in the original email thread):

          It could be feasible to have an interface as Public/Stable for users (anyone can use the API in a stable manner) but Private for implementers. The idea is that it is still a public interface but no third-party code should not subclass or implement it. I suspect a fair amount of hadoop's public interface might fall into that category. That itself is probably an incompatible change, so we might have to wait until after 3.0, however.

          One practical difficulty I see is for test classes. One key need to extend/implement the interface is because users need to provide a mocked test class. If we declare these interfaces are off limits to implement/extend, then they would need to stick with pure mocking via mocking libraries. I'm not sure how onerous that requirement would be.

          cc Steve Loughran for his thoughts.

          Show
          sjlee0 Sangjin Lee added a comment - What if the methods in question are used by rest of YARN code? If someone subclasses this to provide an alternate implementation, calling these methods would lead to RTE. Yes, I agree that it is a very valid concern, as a runtime exception is a poor substitute for a compile-time error. I think a mitigating circumstance here is that these classes are normally extended only by the canonical implementation (protobuf impl): ContainerStatusPBImpl for ContainerStatus , and NodeReportPBImpl for NodeReport . Thus, the real likelihood that these classes would be extended by other classes would be pretty small, but it cannot be ruled out. In practice, the only times these are extended might be test classes (mock classes). In that sense, actually it could be an argument for not moving this change to the trunk. By doing so, we would be extending this window of potential trouble into 3.0. Since 2.8 has not been released, we could consider this as adding compatible default implementation methods which will then be made abstract in 3.0. Thoughts? The reason I am raising this concern is to see if there is a need for one more annotation that clarifies our intent about users extending classes to be plugged in. I do agree that we probably need another audience annotation for whether users may extend/implement classes (which I also proposed in the original email thread): It could be feasible to have an interface as Public/Stable for users (anyone can use the API in a stable manner) but Private for implementers. The idea is that it is still a public interface but no third-party code should not subclass or implement it. I suspect a fair amount of hadoop's public interface might fall into that category. That itself is probably an incompatible change, so we might have to wait until after 3.0, however. One practical difficulty I see is for test classes. One key need to extend/implement the interface is because users need to provide a mocked test class. If we declare these interfaces are off limits to implement/extend, then they would need to stick with pure mocking via mocking libraries. I'm not sure how onerous that requirement would be. cc Steve Loughran for his thoughts.
          Hide
          sjlee0 Sangjin Lee added a comment -

          Ping for more discussions on this. Thanks!

          Show
          sjlee0 Sangjin Lee added a comment - Ping for more discussions on this. Thanks!
          Hide
          stevel@apache.org Steve Loughran added a comment -

          The main reason that a lot of hadoop code switched from interfaces like org.apache.hadoop.mapred.Mapper to (abstract) classes like org.apache.hadoop.mapreduce.Mapper was to make it possible to add new methods without breaking existing code. That's created an obligation: don't break things wherever possible; this JIRA is essentially covering a situation where that wasn't quite met.

          Now people seem to be arguing the other way: if we add new methods, we don't want to provide default implementations, in case people don't implement them. Maybe —but the current policy implemented on these classes broke my mocking test code even though I wasn't using the new methods; again: a failure. To make things worse, I don't think I could even implement the new method in a class which could then be compiled against 2.7.x, as it's parameters were new datatypes. A default implementation wouldn't have had this problem.

          There's no easy answer here. I will note that I have found the implicit policy "break things on public classes as we didn't expect you to implement them" frustrating at times, especially in testing.

          Show
          stevel@apache.org Steve Loughran added a comment - The main reason that a lot of hadoop code switched from interfaces like org.apache.hadoop.mapred.Mapper to (abstract) classes like org.apache.hadoop.mapreduce.Mapper was to make it possible to add new methods without breaking existing code. That's created an obligation: don't break things wherever possible; this JIRA is essentially covering a situation where that wasn't quite met. Now people seem to be arguing the other way: if we add new methods, we don't want to provide default implementations, in case people don't implement them. Maybe —but the current policy implemented on these classes broke my mocking test code even though I wasn't using the new methods; again: a failure. To make things worse, I don't think I could even implement the new method in a class which could then be compiled against 2.7.x, as it's parameters were new datatypes. A default implementation wouldn't have had this problem. There's no easy answer here. I will note that I have found the implicit policy "break things on public classes as we didn't expect you to implement them" frustrating at times, especially in testing.
          Hide
          sjlee0 Sangjin Lee added a comment -

          Thanks for your feedback Steve.

          I don't think there is a disagreement that the API compatibility has been broken and that it needs to be restored before it is released. I also believe that the only way to restore the API compatibility (short of removing the methods) is to add default implementations.

          The proposal on the table is to do just that. The only one more point I'd like to bring up is what to do for 3.0. If we do this for 2.8/2.9, we would have introduced new methods with default implementations for 2.8/2.9. Personally I think it is fine to turn them abstract in 3.0 (the current code). Let me know what you think.

          I'd like to close this soon as this would be a blocker for any 2.x releases as well as 3.0. Thanks!

          Show
          sjlee0 Sangjin Lee added a comment - Thanks for your feedback Steve. I don't think there is a disagreement that the API compatibility has been broken and that it needs to be restored before it is released. I also believe that the only way to restore the API compatibility (short of removing the methods) is to add default implementations. The proposal on the table is to do just that. The only one more point I'd like to bring up is what to do for 3.0. If we do this for 2.8/2.9, we would have introduced new methods with default implementations for 2.8/2.9. Personally I think it is fine to turn them abstract in 3.0 (the current code). Let me know what you think. I'd like to close this soon as this would be a blocker for any 2.x releases as well as 3.0. Thanks!
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I'd rather not make things abstract for 3.x.

          • Every incompatible API change is another barrier to migration. We don't want Hadoop 3 to be python 3.
          • Some of us regularly build against -trunk already; I'm doing that with Spark and the s3guard branch. Incompatible changes really complicate my life
          Show
          stevel@apache.org Steve Loughran added a comment - I'd rather not make things abstract for 3.x. Every incompatible API change is another barrier to migration. We don't want Hadoop 3 to be python 3. Some of us regularly build against -trunk already; I'm doing that with Spark and the s3guard branch. Incompatible changes really complicate my life
          Hide
          sjlee0 Sangjin Lee added a comment -

          Hmm, I definitely understand the desire to minimize incompatibilities even for major versions. On the other hand, it might not be best if we say zero incompatibilities even for a major version upgrade. That would severely limit the ability to make more substantial changes to deliver useful features and get rid of tech debt, and so on. I think you would agree that there needs to be a happy medium here.

          I don't have a strong opinion on this specific one for 3.0. If you feel strongly that this would be a major issue for 3.0, I'd be happy to keep the default methods. Do let me know, and I will update the patches and this JIRA. Thanks!

          Show
          sjlee0 Sangjin Lee added a comment - Hmm, I definitely understand the desire to minimize incompatibilities even for major versions. On the other hand, it might not be best if we say zero incompatibilities even for a major version upgrade. That would severely limit the ability to make more substantial changes to deliver useful features and get rid of tech debt, and so on. I think you would agree that there needs to be a happy medium here. I don't have a strong opinion on this specific one for 3.0. If you feel strongly that this would be a major issue for 3.0, I'd be happy to keep the default methods. Do let me know, and I will update the patches and this JIRA. Thanks!
          Hide
          djp Junping Du added a comment -

          Assign to Sangjin Lee who actively working on this. I think there is no concern for current change on branch-2/branch-2.8 and the only concern is if we want these changes in trunk/branch-3. Given branch-2.8 will be out soon, shall we file a separated JIRA for further discussion on what is right thing to do on trunk?

          Show
          djp Junping Du added a comment - Assign to Sangjin Lee who actively working on this. I think there is no concern for current change on branch-2/branch-2.8 and the only concern is if we want these changes in trunk/branch-3. Given branch-2.8 will be out soon, shall we file a separated JIRA for further discussion on what is right thing to do on trunk?
          Hide
          sjlee0 Sangjin Lee added a comment -

          That's an option. But since this is also a blocker for 3.0.0-alpha2, personally I'd like us to make a call and move on. I'd like your +1/-1 on

          • making these new methods abstract on trunk, or
          • retain the default implementation

          Thoughts?

          Show
          sjlee0 Sangjin Lee added a comment - That's an option. But since this is also a blocker for 3.0.0-alpha2, personally I'd like us to make a call and move on. I'd like your +1/-1 on making these new methods abstract on trunk, or retain the default implementation Thoughts?
          Hide
          djp Junping Du added a comment -

          I think about it again and agree Steve's point is correct. We choose abstract class instead of interface is just to get rid of this situation (breaking compatibility for extended class in source code level). At anytime, we can easily add a default implementation for new added API and we don't have to make them abstract again even we loose compatible restraint in next release. I haven't see any side effort to do so, other than API code don't look so nicely.
          +1 for current branch-2 patch go to trunk also.

          Show
          djp Junping Du added a comment - I think about it again and agree Steve's point is correct. We choose abstract class instead of interface is just to get rid of this situation (breaking compatibility for extended class in source code level). At anytime, we can easily add a default implementation for new added API and we don't have to make them abstract again even we loose compatible restraint in next release. I haven't see any side effort to do so, other than API code don't look so nicely. +1 for current branch-2 patch go to trunk also.
          Hide
          sjlee0 Sangjin Lee added a comment -

          Renamed the patches. The trunk patch should apply cleanly to branch-2 also (2.9).

          Show
          sjlee0 Sangjin Lee added a comment - Renamed the patches. The trunk patch should apply cleanly to branch-2 also (2.9).
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 15m 3s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s 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 mvninstall 10m 16s branch-2.8 passed
          +1 compile 0m 26s branch-2.8 passed with JDK v1.8.0_111
          +1 compile 0m 26s branch-2.8 passed with JDK v1.7.0_121
          +1 checkstyle 0m 16s branch-2.8 passed
          +1 mvnsite 0m 34s branch-2.8 passed
          +1 mvneclipse 0m 17s branch-2.8 passed
          +1 findbugs 1m 27s branch-2.8 passed
          +1 javadoc 0m 21s branch-2.8 passed with JDK v1.8.0_111
          +1 javadoc 0m 23s branch-2.8 passed with JDK v1.7.0_121
          +1 mvninstall 0m 27s the patch passed
          +1 compile 0m 26s the patch passed with JDK v1.8.0_111
          +1 javac 0m 26s the patch passed
          +1 compile 0m 27s the patch passed with JDK v1.7.0_121
          +1 javac 0m 27s the patch passed
          +1 checkstyle 0m 13s the patch passed
          +1 mvnsite 0m 31s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 36s the patch passed
          +1 javadoc 0m 16s the patch passed with JDK v1.8.0_111
          +1 javadoc 0m 19s the patch passed with JDK v1.7.0_121
          +1 unit 0m 27s hadoop-yarn-api in the patch passed with JDK v1.7.0_121.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          37m 12s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:5af2af1
          JIRA Issue YARN-5184
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841806/YARN-5184-branch-2.8.01.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux f6668fc1c8ef 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision branch-2.8 / 802a1fb
          Default Java 1.7.0_121
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_111 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_121
          findbugs v3.0.0
          JDK v1.7.0_121 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/14188/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/14188/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 15m 3s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s 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 mvninstall 10m 16s branch-2.8 passed +1 compile 0m 26s branch-2.8 passed with JDK v1.8.0_111 +1 compile 0m 26s branch-2.8 passed with JDK v1.7.0_121 +1 checkstyle 0m 16s branch-2.8 passed +1 mvnsite 0m 34s branch-2.8 passed +1 mvneclipse 0m 17s branch-2.8 passed +1 findbugs 1m 27s branch-2.8 passed +1 javadoc 0m 21s branch-2.8 passed with JDK v1.8.0_111 +1 javadoc 0m 23s branch-2.8 passed with JDK v1.7.0_121 +1 mvninstall 0m 27s the patch passed +1 compile 0m 26s the patch passed with JDK v1.8.0_111 +1 javac 0m 26s the patch passed +1 compile 0m 27s the patch passed with JDK v1.7.0_121 +1 javac 0m 27s the patch passed +1 checkstyle 0m 13s the patch passed +1 mvnsite 0m 31s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 36s the patch passed +1 javadoc 0m 16s the patch passed with JDK v1.8.0_111 +1 javadoc 0m 19s the patch passed with JDK v1.7.0_121 +1 unit 0m 27s hadoop-yarn-api in the patch passed with JDK v1.7.0_121. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 37m 12s Subsystem Report/Notes Docker Image:yetus/hadoop:5af2af1 JIRA Issue YARN-5184 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841806/YARN-5184-branch-2.8.01.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux f6668fc1c8ef 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision branch-2.8 / 802a1fb Default Java 1.7.0_121 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_111 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_121 findbugs v3.0.0 JDK v1.7.0_121 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/14188/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api Console output https://builds.apache.org/job/PreCommit-YARN-Build/14188/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          djp Junping Du added a comment -

          +1 on patch for branch-2 (and trunk) and branch-2.8.
          Will commit it tomorrow if no other objections.

          Show
          djp Junping Du added a comment - +1 on patch for branch-2 (and trunk) and branch-2.8. Will commit it tomorrow if no other objections.
          Hide
          djp Junping Du added a comment -

          I have commit the patch to trunk, branch-2 and branch-2.8. Thanks Karthik Kambatla for reporting the issue and Sangjin Lee for the fix patch. Also, thanks all for comments and review!

          Show
          djp Junping Du added a comment - I have commit the patch to trunk, branch-2 and branch-2.8. Thanks Karthik Kambatla for reporting the issue and Sangjin Lee for the fix patch. Also, thanks all for comments and review!
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10957 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10957/)
          YARN-5184. Fix up incompatible changes introduced on ContainerStatus and (junping_du: rev a7288da595fdf56c3ccd45c0b6ed2e3efaa043a4)

          • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/ContainerStatus.java
          • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/NodeReport.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10957 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10957/ ) YARN-5184 . Fix up incompatible changes introduced on ContainerStatus and (junping_du: rev a7288da595fdf56c3ccd45c0b6ed2e3efaa043a4) (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/ContainerStatus.java (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/NodeReport.java

            People

            • Assignee:
              sjlee0 Sangjin Lee
              Reporter:
              kasha Karthik Kambatla
            • Votes:
              0 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development