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

Exclude generated protobuf sources from YARN Javadoc build

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Exclude javadocs for proto-generated java classes.

      Description

      Currently YARN build includes Javadoc from generated protobuf sources which is causing CI to fail. This JIRA proposes to exclude generated protobuf sources from YARN Javadoc build

      1. YARN-5132-v1.patch
        0.6 kB
        Subru Krishnan

        Issue Links

          Activity

          Hide
          jianhe Jian He added a comment -

          Subru Krishnan, Karthik Kambatla,

          Billie Rinaldi found this, could you please confirm ? seems like an issue.

          It looks like the maven-javadoc-plugin is not configured properly for the hadoop-yarn module. There are YARN exclusions in the top level pom: https://github.com/apache/hadoop/blob/trunk/pom.xml#L443, and these are overridden in the hadoop-yarn pom: https://github.com/apache/hadoop/blob/trunk/hadoop-yarn-project/hadoop-yarn/pom.xml#L78 by a subset of the package names. I am not sure if the lack of exclusion of the yarn server and yarn webapp packages was intentional or not.

          Show
          jianhe Jian He added a comment - Subru Krishnan , Karthik Kambatla , Billie Rinaldi found this, could you please confirm ? seems like an issue. It looks like the maven-javadoc-plugin is not configured properly for the hadoop-yarn module. There are YARN exclusions in the top level pom: https://github.com/apache/hadoop/blob/trunk/pom.xml#L443 , and these are overridden in the hadoop-yarn pom: https://github.com/apache/hadoop/blob/trunk/hadoop-yarn-project/hadoop-yarn/pom.xml#L78 by a subset of the package names. I am not sure if the lack of exclusion of the yarn server and yarn webapp packages was intentional or not.
          Hide
          subru Subru Krishnan added a comment -

          Thanks Chris Douglas for reviewing and Karthik Kambatla for reviewing/committing!

          Show
          subru Subru Krishnan added a comment - Thanks Chris Douglas for reviewing and Karthik Kambatla for reviewing/committing!
          Hide
          kasha Karthik Kambatla added a comment -

          Thanks Subru Krishnan for the patch, Allen Wittenauer and Chris Douglas for your inputs.

          Just committed this to trunk, branch-2, and branch-2.8.

          Show
          kasha Karthik Kambatla added a comment - Thanks Subru Krishnan for the patch, Allen Wittenauer and Chris Douglas for your inputs. Just committed this to trunk, branch-2, and branch-2.8.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #9883 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9883/)
          YARN-5132. Exclude generated protobuf sources from YARN Javadoc build. (kasha: rev f5ff05cc8a67c125717261392c996136ba66785b)

          • hadoop-yarn-project/hadoop-yarn/pom.xml
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #9883 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9883/ ) YARN-5132 . Exclude generated protobuf sources from YARN Javadoc build. (kasha: rev f5ff05cc8a67c125717261392c996136ba66785b) hadoop-yarn-project/hadoop-yarn/pom.xml
          Hide
          kasha Karthik Kambatla added a comment -

          Checking this in.

          Show
          kasha Karthik Kambatla added a comment - Checking this in.
          Hide
          kasha Karthik Kambatla added a comment -

          +1.

          Will check this in tomorrow (Fri) morning.

          Show
          kasha Karthik Kambatla added a comment - +1. Will check this in tomorrow (Fri) morning.
          Hide
          kasha Karthik Kambatla added a comment - - edited

          I see several reasons to get this in:

          1. As Subru, Chris and others mentioned, I don't see why we should publish javadocs of these proto generated classes. We should actually update the release script to remove javadoc from proto files. Vinod Kumar Vavilapalli, Wangda Tan - you have been working on the releases recently, do we publish javadocs for these?
          2. All JIRAs see these javadoc errors. It is painstakingly hard to figure out what javadoc issues the patch introduces.
          Show
          kasha Karthik Kambatla added a comment - - edited I see several reasons to get this in: As Subru, Chris and others mentioned, I don't see why we should publish javadocs of these proto generated classes. We should actually update the release script to remove javadoc from proto files. Vinod Kumar Vavilapalli , Wangda Tan - you have been working on the releases recently, do we publish javadocs for these? All JIRAs see these javadoc errors. It is painstakingly hard to figure out what javadoc issues the patch introduces.
          Hide
          chris.douglas Chris Douglas added a comment -

          From the bug Subru Krishnan cited, it looks like there's no solution for the javadoc warnings in 2.5, a version we're unlikely to change and Google is unlikely to fix.

          Allen Wittenauer, I think your point is that Jenkins should stop complaining about (new) javadoc warnings in generated code, rather than giving up generating javadoc entirely. The protobuf classes are public APIs, but they're not user-facing in our Java APIs... I'm pretty ambivalent about keeping javadoc for them; including it may mislead someone into writing against them, rather than the API classes. Since (IIRC) we exclude other @Private APIs from the generated javadoc, this seems like a good change, overall. Unless there's a better way to effect it?

          Show
          chris.douglas Chris Douglas added a comment - From the bug Subru Krishnan cited, it looks like there's no solution for the javadoc warnings in 2.5, a version we're unlikely to change and Google is unlikely to fix. Allen Wittenauer , I think your point is that Jenkins should stop complaining about (new) javadoc warnings in generated code, rather than giving up generating javadoc entirely. The protobuf classes are public APIs, but they're not user-facing in our Java APIs... I'm pretty ambivalent about keeping javadoc for them; including it may mislead someone into writing against them, rather than the API classes. Since (IIRC) we exclude other @Private APIs from the generated javadoc, this seems like a good change, overall. Unless there's a better way to effect it?
          Hide
          subru Subru Krishnan added a comment -

          Arun Suresh, thanks for providing the clarification.

          Allen Wittenauer, I feel a few points are being missed:

          • The exclusion is confined to protobuf generated internal classes.
          • Protobuf generation has a known Javadoc bug with JDK8 which is open. Once the bug is resolved and we upgrade to an appropriate version, we can remove this exclusion.
          • The change aligns YARN with HDFS project.
          Show
          subru Subru Krishnan added a comment - Arun Suresh , thanks for providing the clarification. Allen Wittenauer , I feel a few points are being missed: The exclusion is confined to protobuf generated internal classes . Protobuf generation has a known Javadoc bug with JDK8 which is open. Once the bug is resolved and we upgrade to an appropriate version, we can remove this exclusion. The change aligns YARN with HDFS project .
          Hide
          aw Allen Wittenauer added a comment -

          ANY documentation is better than NO documentation. Removing the javadoc effectively makes these APIs disappear from search results.

          Show
          aw Allen Wittenauer added a comment - ANY documentation is better than NO documentation. Removing the javadoc effectively makes these APIs disappear from search results.
          Hide
          asuresh Arun Suresh added a comment - - edited

          then that means that non-Java clients are basically screwed. It also makes the Go example by Hortonworks extremely problematic.

          Allen Wittenauer, appoligize but I do NOT see the connection between generated java classes and go clients. A go client would generate its own go files from the protobuf and use it.

          Show
          asuresh Arun Suresh added a comment - - edited then that means that non-Java clients are basically screwed. It also makes the Go example by Hortonworks extremely problematic. Allen Wittenauer , appoligize but I do NOT see the connection between generated java classes and go clients. A go client would generate its own go files from the protobuf and use it.
          Hide
          aw Allen Wittenauer added a comment -

          No, we do not in YARN.

          Citation needed, because if that's the case, then that means that non-Java clients are basically screwed. It also makes the Go example by Hortonworks extremely problematic. (http://hortonworks.com/blog/go-hadoop-err-hadoop-and-go/)

          Show
          aw Allen Wittenauer added a comment - No, we do not in YARN. Citation needed, because if that's the case, then that means that non-Java clients are basically screwed. It also makes the Go example by Hortonworks extremely problematic. ( http://hortonworks.com/blog/go-hadoop-err-hadoop-and-go/ )
          Hide
          subru Subru Krishnan added a comment -

          No, we do not in YARN. They are behind interfaces like ApplicationClientProtocol which are the public APIs. HDFS already does the exact same exclusion as Chris Nauroth pointed out earlier. Additionally there is no means to generate Javadocs for protobufs currently.

          Show
          subru Subru Krishnan added a comment - No, we do not in YARN. They are behind interfaces like ApplicationClientProtocol which are the public APIs. HDFS already does the exact same exclusion as Chris Nauroth pointed out earlier. Additionally there is no means to generate Javadocs for protobufs currently .
          Hide
          aw Allen Wittenauer added a comment -

          I'm not sure if this is really the correct thing to do. Don't we consider some of the protobuf RPCs as a public interface?

          Show
          aw Allen Wittenauer added a comment - I'm not sure if this is really the correct thing to do. Don't we consider some of the protobuf RPCs as a public interface?
          Hide
          subru Subru Krishnan added a comment -

          There are no unit tests as the patch has only pom file changes. Consequently the failing tests are unrelated and are covered by YARN-5091.

          Show
          subru Subru Krishnan added a comment - There are no unit tests as the patch has only pom file changes. Consequently the failing tests are unrelated and are covered by YARN-5091 .
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 12s 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 19s trunk passed
          +1 compile 1m 59s trunk passed
          +1 mvnsite 2m 29s trunk passed
          +1 mvneclipse 0m 18s trunk passed
          +1 javadoc 1m 57s trunk passed
          +1 mvninstall 1m 58s the patch passed
          +1 compile 1m 58s the patch passed
          +1 javac 1m 58s the patch passed
          +1 mvnsite 2m 28s the patch passed
          +1 mvneclipse 0m 16s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 javadoc 1m 34s hadoop-yarn-project_hadoop-yarn generated 0 new + 6617 unchanged - 7318 fixed = 6617 total (was 13935)
          +1 javadoc 1m 15s hadoop-yarn in the patch passed.
          -1 unit 54m 33s hadoop-yarn in the patch failed.
          +1 asflicense 0m 17s Patch does not generate ASF License warnings.
          76m 33s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.applicationsmanager.TestAMRestart
            hadoop.yarn.server.resourcemanager.TestClientRMTokens
            hadoop.yarn.server.resourcemanager.TestAMAuthorization



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:2c91fd8
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12805793/YARN-5132-v1.patch
          JIRA Issue YARN-5132
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml
          uname Linux a0b2da07675b 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 trunk / b4078bd
          Default Java 1.8.0_91
          unit https://builds.apache.org/job/PreCommit-YARN-Build/11646/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn.txt
          unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/11646/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/11646/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/11646/console
          Powered by Apache Yetus 0.2.0 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 12s 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 19s trunk passed +1 compile 1m 59s trunk passed +1 mvnsite 2m 29s trunk passed +1 mvneclipse 0m 18s trunk passed +1 javadoc 1m 57s trunk passed +1 mvninstall 1m 58s the patch passed +1 compile 1m 58s the patch passed +1 javac 1m 58s the patch passed +1 mvnsite 2m 28s the patch passed +1 mvneclipse 0m 16s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 javadoc 1m 34s hadoop-yarn-project_hadoop-yarn generated 0 new + 6617 unchanged - 7318 fixed = 6617 total (was 13935) +1 javadoc 1m 15s hadoop-yarn in the patch passed. -1 unit 54m 33s hadoop-yarn in the patch failed. +1 asflicense 0m 17s Patch does not generate ASF License warnings. 76m 33s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.applicationsmanager.TestAMRestart   hadoop.yarn.server.resourcemanager.TestClientRMTokens   hadoop.yarn.server.resourcemanager.TestAMAuthorization Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12805793/YARN-5132-v1.patch JIRA Issue YARN-5132 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml uname Linux a0b2da07675b 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 trunk / b4078bd Default Java 1.8.0_91 unit https://builds.apache.org/job/PreCommit-YARN-Build/11646/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/11646/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/11646/testReport/ modules C: hadoop-yarn-project/hadoop-yarn U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/11646/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.
          Hide
          subru Subru Krishnan added a comment - - edited

          Adding an exclusion for protobuf-generated sources from YARN build as suggested by Chris Nauroth.

          I ran test-patch locally with and without this change for YARN-4887 to verify it works correctly.

          Show
          subru Subru Krishnan added a comment - - edited Adding an exclusion for protobuf-generated sources from YARN build as suggested by Chris Nauroth . I ran test-patch locally with and without this change for YARN-4887 to verify it works correctly.

            People

            • Assignee:
              subru Subru Krishnan
              Reporter:
              subru Subru Krishnan
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development