Details

    • Type: Test
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0-alpha1
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: build, test
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Hide
      <!-- markdown -->
      * test-patch.sh now has new output that is different than the previous versions
      * test-patch.sh is now pluggable via the test-patch.d directory, with checkstyle and shellcheck tests included
      * JIRA comments now use much more markup to improve readability
      * test-patch.sh now supports either a file name, a URL, or a JIRA issue as input in developer mode
      * If part of the patch testing code is changed, test-patch.sh will now attempt to re-executing itself using the new version.
      * Some logic to try and reduce the amount of unnecessary tests. For example, patches that only modify markdown should not run the Java compilation tests.
      * Plugins for checkstyle, shellcheck, and whitespace now execute as necessary.
      * New test code for mvn site
      * A breakdown of the times needed to execute certain blocks as well as a total runtime is now reported to assist in fixing long running tests and optimize the entire process.
      * Several new options
        * --resetrepo will put test-patch.sh in destructive mode, similar to a normal Jenkins run
        * --testlist allows one to provide a comma delimited list of test subsystems to forcibly execute
        * --modulelist to provide a comma delimited list of module tests to execute in addition to the ones that are automatically detected
        * --offline mode to attempt to stop connecting to the Internet for certain operations
      * test-patch.sh now defaults to the POSIX equivalents on Solaris and Illumos-based operating systems
      * shelldocs.py may be used to generate test-patch.sh API information
      * FindBugs output is now listed on the JIRA comment
      * lots of general code cleanup, including attempts to remove any local state files to reduce potential race conditions
      * Some logic to determine if a patch is for a given major branch using several strategies as well as a particular git ref (using git+ref as part of the name).
      * Some logic to determine if a patch references a particular JIRA issue.
      * Unit tests are only flagged as necessary with native or Java code, since Hadoop has no framework in place yet for other types of unit tests.
      * test-patch now exits with a failure status if problems arise trying to do git checkouts. Previously the exit code was success.
      Show
      <!-- markdown --> * test-patch.sh now has new output that is different than the previous versions * test-patch.sh is now pluggable via the test-patch.d directory, with checkstyle and shellcheck tests included * JIRA comments now use much more markup to improve readability * test-patch.sh now supports either a file name, a URL, or a JIRA issue as input in developer mode * If part of the patch testing code is changed, test-patch.sh will now attempt to re-executing itself using the new version. * Some logic to try and reduce the amount of unnecessary tests. For example, patches that only modify markdown should not run the Java compilation tests. * Plugins for checkstyle, shellcheck, and whitespace now execute as necessary. * New test code for mvn site * A breakdown of the times needed to execute certain blocks as well as a total runtime is now reported to assist in fixing long running tests and optimize the entire process. * Several new options   * --resetrepo will put test-patch.sh in destructive mode, similar to a normal Jenkins run   * --testlist allows one to provide a comma delimited list of test subsystems to forcibly execute   * --modulelist to provide a comma delimited list of module tests to execute in addition to the ones that are automatically detected   * --offline mode to attempt to stop connecting to the Internet for certain operations * test-patch.sh now defaults to the POSIX equivalents on Solaris and Illumos-based operating systems * shelldocs.py may be used to generate test-patch.sh API information * FindBugs output is now listed on the JIRA comment * lots of general code cleanup, including attempts to remove any local state files to reduce potential race conditions * Some logic to determine if a patch is for a given major branch using several strategies as well as a particular git ref (using git+ref as part of the name). * Some logic to determine if a patch references a particular JIRA issue. * Unit tests are only flagged as necessary with native or Java code, since Hadoop has no framework in place yet for other types of unit tests. * test-patch now exits with a failure status if problems arise trying to do git checkouts. Previously the exit code was success.

      Description

      This code is bad and you should feel bad.

      1. HADOOP-11746-21.patch
        112 kB
        Allen Wittenauer
      2. HADOOP-11746-21.branch-2.patch
        106 kB
        Allen Wittenauer
      3. HADOOP-11746-20.patch
        112 kB
        Allen Wittenauer
      4. HADOOP-11746-19.patch
        112 kB
        Allen Wittenauer
      5. HADOOP-11746-18.patch
        110 kB
        Allen Wittenauer
      6. HADOOP-11746-17.patch
        109 kB
        Allen Wittenauer
      7. HADOOP-11746-16.patch
        109 kB
        Allen Wittenauer
      8. HADOOP-11746-15.patch
        110 kB
        Allen Wittenauer
      9. HADOOP-11746-14.patch
        110 kB
        Allen Wittenauer
      10. HADOOP-11746-13.patch
        108 kB
        Allen Wittenauer
      11. HADOOP-11746-12.patch
        108 kB
        Allen Wittenauer
      12. HADOOP-11746-11.patch
        106 kB
        Allen Wittenauer
      13. HADOOP-11746-10.patch
        105 kB
        Allen Wittenauer
      14. HADOOP-11746-09.patch
        100 kB
        Allen Wittenauer
      15. HADOOP-11746-07.patch
        96 kB
        Allen Wittenauer
      16. HADOOP-11746-06.patch
        96 kB
        Allen Wittenauer
      17. HADOOP-11746-05.patch
        94 kB
        Allen Wittenauer
      18. HADOOP-11746-04.patch
        82 kB
        Allen Wittenauer
      19. HADOOP-11746-03.patch
        78 kB
        Allen Wittenauer
      20. HADOOP-11746-02.patch
        64 kB
        Allen Wittenauer
      21. HADOOP-11746-01.patch
        63 kB
        Allen Wittenauer
      22. HADOOP-11746-00.patch
        44 kB
        Allen Wittenauer

        Issue Links

          Activity

          Hide
          cnauroth Chris Nauroth added a comment -

          Allen Wittenauer, I noticed a small discrepancy in CHANGES.txt between trunk and branch-2. On trunk, HADOOP-11746 is listed in the Incompatible Changes section. On branch-2, it's in New Features. Would you mind committing a change to make them consistent?

          I'd suggest New Features. I mentioned in an earlier comment that I don't think the compatibility guidelines apply to CI automation that doesn't ship with the product. If you feel strongly about marking it incompatible though, I'm still +1 to moving it to the Incompatible Changes section. Thanks!

          Show
          cnauroth Chris Nauroth added a comment - Allen Wittenauer , I noticed a small discrepancy in CHANGES.txt between trunk and branch-2. On trunk, HADOOP-11746 is listed in the Incompatible Changes section. On branch-2, it's in New Features. Would you mind committing a change to make them consistent? I'd suggest New Features. I mentioned in an earlier comment that I don't think the compatibility guidelines apply to CI automation that doesn't ship with the product. If you feel strongly about marking it incompatible though, I'm still +1 to moving it to the Incompatible Changes section. Thanks!
          Hide
          aw Allen Wittenauer added a comment -

          Just a heads up, I've posted an addendum patch to HADOOP-11861 that fixes some semi-critical bugs based upon the past 24 hours of Jenkins usage.

          Show
          aw Allen Wittenauer added a comment - Just a heads up, I've posted an addendum patch to HADOOP-11861 that fixes some semi-critical bugs based upon the past 24 hours of Jenkins usage.
          Hide
          aw Allen Wittenauer added a comment -

          Thoughts?

          I think ignoring whitespace problems when pointed out is lazy, given how relatively easy they are to fix. At this point, both git and test-patch are now warning about this issue. If one is unmotivated to fix such a simple problem, what else is getting ignored?

          HowToContribute is actually pretty explicit about checkstyle errors. Patches that violate it are supposed to be rejected. We've been lax in the past. It's time to step up our vigilance and this is the first step. We certainly need to improve the output here, but at least we're getting notified about them.

          Show
          aw Allen Wittenauer added a comment - Thoughts? I think ignoring whitespace problems when pointed out is lazy, given how relatively easy they are to fix. At this point, both git and test-patch are now warning about this issue. If one is unmotivated to fix such a simple problem, what else is getting ignored? HowToContribute is actually pretty explicit about checkstyle errors. Patches that violate it are supposed to be rejected. We've been lax in the past. It's time to step up our vigilance and this is the first step. We certainly need to improve the output here, but at least we're getting notified about them.
          Hide
          busbey Sean Busbey added a comment -

          The QA bot doesn't get a binding vote, so I think clearly stating -1 for things that violate the community guidelines is preferable. Individual committers can always choose to ignore build bot feedback.

          The whitespace / checkstyle improvements are being tracked on HADOOP-11866. please keep the discussion of them over there.

          Show
          busbey Sean Busbey added a comment - The QA bot doesn't get a binding vote, so I think clearly stating -1 for things that violate the community guidelines is preferable. Individual committers can always choose to ignore build bot feedback. The whitespace / checkstyle improvements are being tracked on HADOOP-11866 . please keep the discussion of them over there.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Thanks Allen Wittenauer for this great work, adding checkstyle/whitespace check is helpful, but I think it's better to indicate it's a -0/+0 instead of "-1". Different from other checks like findbugs/javac WARNINGs/test-not-include, they're best-to-have minor format suggestions.

          An example is: https://issues.apache.org/jira/browse/YARN-3413?focusedCommentId=14507511&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14507511.

          Thoughts?

          Show
          leftnoteasy Wangda Tan added a comment - Thanks Allen Wittenauer for this great work, adding checkstyle/whitespace check is helpful, but I think it's better to indicate it's a -0/+0 instead of "-1". Different from other checks like findbugs/javac WARNINGs/test-not-include, they're best-to-have minor format suggestions. An example is: https://issues.apache.org/jira/browse/YARN-3413?focusedCommentId=14507511&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14507511 . Thoughts?
          Hide
          aw Allen Wittenauer added a comment -

          Linking HADOOP-11861, which I've stuffed a few fixes.

          Show
          aw Allen Wittenauer added a comment - Linking HADOOP-11861 , which I've stuffed a few fixes.
          Hide
          aw Allen Wittenauer added a comment -

          I originally had links to the checkstyle reports. Then realized they were several megabytes in size.... basically, too big to stuff into a web browser (at least, by random people clicking on it.. surprise!) plus I was worried about the load on the jenkins' hosts .

          I tried a few different strategies to make checkstyle more usable, but it frankly isn't built for this type of thing. I opted to steal the HBase code and punt on it. On the flip side, it should be possible for someone to generate their own checkstyle report, dig up the file that broke, and fix it.

          Show
          aw Allen Wittenauer added a comment - I originally had links to the checkstyle reports. Then realized they were several megabytes in size.... basically, too big to stuff into a web browser (at least, by random people clicking on it.. surprise!) plus I was worried about the load on the jenkins' hosts . I tried a few different strategies to make checkstyle more usable, but it frankly isn't built for this type of thing. I opted to steal the HBase code and punt on it. On the flip side, it should be possible for someone to generate their own checkstyle report, dig up the file that broke, and fix it.
          Hide
          Naganarasimha Naganarasimha G R added a comment -

          i have already raised one followup jira HADOOP-11866 will add this point to it...

          Show
          Naganarasimha Naganarasimha G R added a comment - i have already raised one followup jira HADOOP-11866 will add this point to it...
          Hide
          busbey Sean Busbey added a comment -

          Those sounds like fine things for a follow-on improvement jira.

          Show
          busbey Sean Busbey added a comment - Those sounds like fine things for a follow-on improvement jira.
          Hide
          Naganarasimha Naganarasimha G R added a comment -

          thanks for the reply Sean Busbey,
          In that case would it be better to have header some thing like "Name of the modified Java file | Number of Previous check style issues | Number of Current check style issues" ? and also if possible print the links of previous and current check style output. (either in the report or in the output file)

          Show
          Naganarasimha Naganarasimha G R added a comment - thanks for the reply Sean Busbey , In that case would it be better to have header some thing like "Name of the modified Java file | Number of Previous check style issues | Number of Current check style issues" ? and also if possible print the links of previous and current check style output. (either in the report or in the output file)
          Hide
          busbey Sean Busbey added a comment -

          First is number of reported errors in that file pre-patch, second is the number after.

          Getting useful pointers from checkstyle is difficult. It's relatively high on the list of improvements for HBase (we do this file-level count right now as well). When that happens we'll push it back over into Hadoop.

          Show
          busbey Sean Busbey added a comment - First is number of reported errors in that file pre-patch, second is the number after. Getting useful pointers from checkstyle is difficult. It's relatively high on the list of improvements for HBase (we do this file-level count right now as well). When that happens we'll push it back over into Hadoop.
          Hide
          Naganarasimha Naganarasimha G R added a comment -

          Hi Allen Wittenauer,
          Sorry for the naive query but some how dint understand the output of the check style. For instance in yarn-2740 output comes as

          hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java 601 604
          hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/nodelabels/NodeLabelsStore.java 32 33
          hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/nodelabels/FileSystemNodeLabelsStore.java 75 77
          hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/nodelabels/CommonNodeLabelsManager.java 240 241
          hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/AdminService.java 146 151

          whats the significance of the 2 integers which follows the modified file name ?

          Show
          Naganarasimha Naganarasimha G R added a comment - Hi Allen Wittenauer , Sorry for the naive query but some how dint understand the output of the check style. For instance in yarn-2740 output comes as hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java 601 604 hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/nodelabels/NodeLabelsStore.java 32 33 hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/nodelabels/FileSystemNodeLabelsStore.java 75 77 hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/nodelabels/CommonNodeLabelsManager.java 240 241 hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/AdminService.java 146 151 whats the significance of the 2 integers which follows the modified file name ?
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk #2121 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2121/)
          HADOOP-11746. rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0)

          • dev-support/shelldocs.py
          • dev-support/test-patch.d/whitespace.sh
          • dev-support/test-patch.sh
          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/test-patch.d/checkstyle.sh
          • dev-support/test-patch.d/shellcheck.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #2121 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2121/ ) HADOOP-11746 . rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0) dev-support/shelldocs.py dev-support/test-patch.d/whitespace.sh dev-support/test-patch.sh hadoop-common-project/hadoop-common/CHANGES.txt dev-support/test-patch.d/checkstyle.sh dev-support/test-patch.d/shellcheck.sh
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #172 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/172/)
          HADOOP-11746. rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0)

          • dev-support/test-patch.d/whitespace.sh
          • dev-support/shelldocs.py
          • dev-support/test-patch.d/checkstyle.sh
          • dev-support/test-patch.sh
          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/test-patch.d/shellcheck.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #172 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/172/ ) HADOOP-11746 . rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0) dev-support/test-patch.d/whitespace.sh dev-support/shelldocs.py dev-support/test-patch.d/checkstyle.sh dev-support/test-patch.sh hadoop-common-project/hadoop-common/CHANGES.txt dev-support/test-patch.d/shellcheck.sh
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Yarn-trunk #905 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/905/)
          HADOOP-11746. rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0)

          • dev-support/test-patch.sh
          • dev-support/shelldocs.py
          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/test-patch.d/whitespace.sh
          • dev-support/test-patch.d/shellcheck.sh
          • dev-support/test-patch.d/checkstyle.sh
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #905 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/905/ ) HADOOP-11746 . rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0) dev-support/test-patch.sh dev-support/shelldocs.py hadoop-common-project/hadoop-common/CHANGES.txt dev-support/test-patch.d/whitespace.sh dev-support/test-patch.d/shellcheck.sh dev-support/test-patch.d/checkstyle.sh
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #171 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/171/)
          HADOOP-11746. rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0)

          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/test-patch.sh
          • dev-support/test-patch.d/shellcheck.sh
          • dev-support/shelldocs.py
          • dev-support/test-patch.d/whitespace.sh
          • dev-support/test-patch.d/checkstyle.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #171 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/171/ ) HADOOP-11746 . rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0) hadoop-common-project/hadoop-common/CHANGES.txt dev-support/test-patch.sh dev-support/test-patch.d/shellcheck.sh dev-support/shelldocs.py dev-support/test-patch.d/whitespace.sh dev-support/test-patch.d/checkstyle.sh
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #162 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/162/)
          HADOOP-11746. rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0)

          • dev-support/test-patch.d/shellcheck.sh
          • dev-support/test-patch.sh
          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/shelldocs.py
          • dev-support/test-patch.d/whitespace.sh
          • dev-support/test-patch.d/checkstyle.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #162 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/162/ ) HADOOP-11746 . rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0) dev-support/test-patch.d/shellcheck.sh dev-support/test-patch.sh hadoop-common-project/hadoop-common/CHANGES.txt dev-support/shelldocs.py dev-support/test-patch.d/whitespace.sh dev-support/test-patch.d/checkstyle.sh
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #2103 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2103/)
          HADOOP-11746. rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0)

          • dev-support/test-patch.d/whitespace.sh
          • dev-support/test-patch.d/checkstyle.sh
          • dev-support/shelldocs.py
          • dev-support/test-patch.sh
          • dev-support/test-patch.d/shellcheck.sh
          • hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2103 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2103/ ) HADOOP-11746 . rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0) dev-support/test-patch.d/whitespace.sh dev-support/test-patch.d/checkstyle.sh dev-support/shelldocs.py dev-support/test-patch.sh dev-support/test-patch.d/shellcheck.sh hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          aw Allen Wittenauer added a comment - - edited

          I posted this on -dev, but to copy the status update here:

          • So far, HADOOP-11861 was filed which is luckily an extremely easy bug to fix.
          • There have been a few runs which seems to indicate that something is destroying the artifact directory in the middle of runs…. which is very very odd and something I hadn’t seen in any of my testing. In any case, I clearly need to add some safety code here to report back that something went awry and report back which node, console, etc this happened on. Someone more familiar with the Jenkins setup might be able to shed some light on why that might happen. All of these runs appear to be on H3, so might be related? Impacted issues with this have been:
          • Some sort of good news! The new mvn site test does appear to be capable of catching issues! YARN-3410 was committed just prior to the new test code and has markdown-to-html syntax issues. HADOOP-11627 (for example) shows the results.
          Show
          aw Allen Wittenauer added a comment - - edited I posted this on -dev, but to copy the status update here: So far, HADOOP-11861 was filed which is luckily an extremely easy bug to fix. There have been a few runs which seems to indicate that something is destroying the artifact directory in the middle of runs…. which is very very odd and something I hadn’t seen in any of my testing. In any case, I clearly need to add some safety code here to report back that something went awry and report back which node, console, etc this happened on. Someone more familiar with the Jenkins setup might be able to shed some light on why that might happen. All of these runs appear to be on H3, so might be related? Impacted issues with this have been: HDFS-8200 ( https://builds.apache.org/job/PreCommit-HDFS-Build/10335/ ) HDFS-8147 ( https://builds.apache.org/job/PreCommit-HDFS-Build/10338/ ) YARN-3301 ( https://builds.apache.org/job/PreCommit-YARN-Build/7441/ ) Some sort of good news! The new mvn site test does appear to be capable of catching issues! YARN-3410 was committed just prior to the new test code and has markdown-to-html syntax issues. HADOOP-11627 (for example) shows the results.
          Hide
          aw Allen Wittenauer added a comment -

          Nope, MAPREDUCE-6324 @ https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5426/console was first by a few minutes.

          Both have entered into mvn test phase. Interesting to note that git clean removed quite a bit of gunk from the git repo on the HDFS test. Hmmmmmmmmmm......

          Show
          aw Allen Wittenauer added a comment - Nope, MAPREDUCE-6324 @ https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5426/console was first by a few minutes. Both have entered into mvn test phase. Interesting to note that git clean removed quite a bit of gunk from the git repo on the HDFS test. Hmmmmmmmmmm......
          Hide
          aw Allen Wittenauer added a comment -

          FYI, HDFS-8200 @ https://builds.apache.org/job/PreCommit-HDFS-Build/10335/console appears to be the first jenkins run with the new test-patch.sh .

          Show
          aw Allen Wittenauer added a comment - FYI, HDFS-8200 @ https://builds.apache.org/job/PreCommit-HDFS-Build/10335/console appears to be the first jenkins run with the new test-patch.sh .
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #7627 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7627/)
          HADOOP-11746. rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0)

          • dev-support/test-patch.d/checkstyle.sh
          • dev-support/shelldocs.py
          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/test-patch.sh
          • dev-support/test-patch.d/shellcheck.sh
          • dev-support/test-patch.d/whitespace.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #7627 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7627/ ) HADOOP-11746 . rewrite test-patch.sh (aw) (aw: rev 73ddb6b4f825be1d06fd1d2be86a4bea241e7aa0) dev-support/test-patch.d/checkstyle.sh dev-support/shelldocs.py hadoop-common-project/hadoop-common/CHANGES.txt dev-support/test-patch.sh dev-support/test-patch.d/shellcheck.sh dev-support/test-patch.d/whitespace.sh
          Hide
          aw Allen Wittenauer added a comment -

          This has been committed to trunk and branch-2.

          Thanks everyone for the assistance!

          Show
          aw Allen Wittenauer added a comment - This has been committed to trunk and branch-2. Thanks everyone for the assistance!
          Hide
          aw Allen Wittenauer added a comment -

          Yeah, I was surprised too! I did indeed just copy the code over the top of it. Hopefully it works. Haha.

          Show
          aw Allen Wittenauer added a comment - Yeah, I was surprised too! I did indeed just copy the code over the top of it. Hopefully it works. Haha.
          Hide
          cnauroth Chris Nauroth added a comment -

          I didn't realize trunk test-patch.sh had diverged so much from branch-2. Is this basically just copying the new code over to branch-2? If so, then +1 for the branch-2 patch. If not, then can you point out specific bits that would need review? Thanks!

          Show
          cnauroth Chris Nauroth added a comment - I didn't realize trunk test-patch.sh had diverged so much from branch-2. Is this basically just copying the new code over to branch-2? If so, then +1 for the branch-2 patch. If not, then can you point out specific bits that would need review? Thanks!
          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/12726970/HADOOP-11746-21.patch
          against trunk revision 997408e.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 4 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6141//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6141//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/12726970/HADOOP-11746-21.patch against trunk revision 997408e. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6141//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6141//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          branch-2 version.

          Show
          aw Allen Wittenauer added a comment - branch-2 version.
          Hide
          cnauroth Chris Nauroth added a comment -

          I checked the diff since my last review (-17), and everything looks good to me. I agree with the decision to maintain the existing behavior of -1 when the existing trunk build is already broken, and I agree with changing those failure code paths to exit with a non-zero code.

          +1 for patch -21. Thanks for your work on this, Allen. The new functionality is great. Now hurry up and commit before you get more ideas! Sean, thank you for helping with the code review.

          Show
          cnauroth Chris Nauroth added a comment - I checked the diff since my last review (-17), and everything looks good to me. I agree with the decision to maintain the existing behavior of -1 when the existing trunk build is already broken, and I agree with changing those failure code paths to exit with a non-zero code. +1 for patch -21. Thanks for your work on this, Allen. The new functionality is great. Now hurry up and commit before you get more ideas! Sean, thank you for helping with the code review.
          Hide
          aw Allen Wittenauer added a comment -

          -21:
          A one line change in the site test:

          < +  add_jira_table +1 site "There were no new javadoc warning messages."
          ---
          > +  add_jira_table +1 site "Site still builds."
          

          ...

          Chris Nauroth, does your +1 still stand? I'm pretty much ready to commit this. Promise. lol

          Show
          aw Allen Wittenauer added a comment - -21: A one line change in the site test: < + add_jira_table +1 site "There were no new javadoc warning messages." --- > + add_jira_table +1 site "Site still builds." ... Chris Nauroth , does your +1 still stand? I'm pretty much ready to commit this. Promise. lol
          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/12726614/HADOOP-11746-20.patch
          against trunk revision f967fd2.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 4 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6129//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6129//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/12726614/HADOOP-11746-20.patch against trunk revision f967fd2. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6129//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6129//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          Yeah I think it was probably a mistake.

          Here's -20 which changes all of those to return 1, thus failing the jenkins build. I guess we'll see what happens.

          Show
          aw Allen Wittenauer added a comment - Yeah I think it was probably a mistake. Here's -20 which changes all of those to return 1, thus failing the jenkins build. I guess we'll see what happens.
          Hide
          busbey Sean Busbey added a comment -

          probably an oversight? exiting with 0 in those conditions would prevent the jenkins job from registering as a failure, but I'm not sure why that would be desirable.

          Show
          busbey Sean Busbey added a comment - probably an oversight? exiting with 0 in those conditions would prevent the jenkins job from registering as a failure, but I'm not sure why that would be desirable.
          Hide
          aw Allen Wittenauer added a comment -

          One thing I'm still confused by is why the original test-patch exit'd with 0 during some failure situations. Is this a bug in the original or was there a reason? I kept that behavior but it still seems wrong...

          I asked this question way above, but haven't gotten an answer. This still feels wrong to me and suspect these should really be exiting with 1.

          Show
          aw Allen Wittenauer added a comment - One thing I'm still confused by is why the original test-patch exit'd with 0 during some failure situations. Is this a bug in the original or was there a reason? I kept that behavior but it still seems wrong... I asked this question way above, but haven't gotten an answer. This still feels wrong to me and suspect these should really be exiting with 1.
          Hide
          aw Allen Wittenauer added a comment - - edited

          I found that surprising, as compared to say "-0" since in those cases the QA bot can't judge the suitability of the patch.

          This is actually how the old test-patch.sh works as well. I tend to agree with the -1 because if the base universe is broken, it really can't judge how well the patch is going to work either. The +1's are going to be false flags.

          The rewrite is a great improvement. Any idea what else you want to cover before pushing?

          Thanks! Not really planning on more changes. Playing around with HADOOP-11843 (which upgraded shellcheck) and realizing I hadn't exercised the site tests yet popped up the problems fixed in -19. At this point, I think all the subsystems have been thoroughly worked (at least by me) so any outstanding issues will likely be edge case bugs and/or issues with the Jenkins build environment.

          [There are one or two more optimizations I could make in site tests, but since those run so quickly anyway, they aren't a big concern.]

          Show
          aw Allen Wittenauer added a comment - - edited I found that surprising, as compared to say "-0" since in those cases the QA bot can't judge the suitability of the patch. This is actually how the old test-patch.sh works as well. I tend to agree with the -1 because if the base universe is broken, it really can't judge how well the patch is going to work either. The +1's are going to be false flags. The rewrite is a great improvement. Any idea what else you want to cover before pushing? Thanks! Not really planning on more changes. Playing around with HADOOP-11843 (which upgraded shellcheck) and realizing I hadn't exercised the site tests yet popped up the problems fixed in -19. At this point, I think all the subsystems have been thoroughly worked (at least by me) so any outstanding issues will likely be edge case bugs and/or issues with the Jenkins build environment. [There are one or two more optimizations I could make in site tests, but since those run so quickly anyway, they aren't a big concern.]
          Hide
          busbey Sean Busbey added a comment -

          This is a nit / stylistic issue, but I noticed that if compilation, javadoc, site, or checkstyle are broken before applying the patch, the jira comment is -1. I found that surprising, as compared to say "-0" since in those cases the QA bot can't judge the suitability of the patch.

          The rewrite is a great improvement. Any idea what else you want to cover before pushing?

          Show
          busbey Sean Busbey added a comment - This is a nit / stylistic issue, but I noticed that if compilation, javadoc, site, or checkstyle are broken before applying the patch, the jira comment is -1. I found that surprising, as compared to say "-0" since in those cases the QA bot can't judge the suitability of the patch. The rewrite is a great improvement. Any idea what else you want to cover before pushing?
          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/12726461/HADOOP-11746-19.patch
          against trunk revision 5459b24.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 4 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6123//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6123//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/12726461/HADOOP-11746-19.patch against trunk revision 5459b24. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6123//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6123//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment - - edited

          -19:

          • Merged in the fixes from HADOOP-11845, otherwise rat checks fail because mvn site generates a file that violates rat. this also enables shelldocs to actually build documentation for test-patch's API too.
          • shellcheck test reports version of shellcheck
          • shellcheck find command was very broken
          • upgraded my local copy of shellcheck, which now includes some new checks. fixed errors from those new checks (mainly local blah=subshell issues)
          • somewhere along the way, the value of the initial cwd got screwed up.
          • re-arranged the findbugs code a bit to give more realistic times
          • if for some reason openssl or base64 throws an error, just send them to /dev/null
          • fixed a minor message bug in the site check
          Show
          aw Allen Wittenauer added a comment - - edited -19: Merged in the fixes from HADOOP-11845 , otherwise rat checks fail because mvn site generates a file that violates rat. this also enables shelldocs to actually build documentation for test-patch's API too. shellcheck test reports version of shellcheck shellcheck find command was very broken upgraded my local copy of shellcheck, which now includes some new checks. fixed errors from those new checks (mainly local blah=subshell issues) somewhere along the way, the value of the initial cwd got screwed up. re-arranged the findbugs code a bit to give more realistic times if for some reason openssl or base64 throws an error, just send them to /dev/null fixed a minor message bug in the site check
          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/12726255/HADOOP-11746-18.patch
          against trunk revision d573f09.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 4 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6118//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6118//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/12726255/HADOOP-11746-18.patch against trunk revision d573f09. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6118//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6118//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment - - edited

          -18:

          • add --modulelist and --testlist options to forcibly add modules and test subsystems to a run. there is no way to delete, but that's probably a good thing really.
          • removed the branch restrictions since this apparently won't impact jenkins
          • the check for new tests actually uses the /test/ file path against the list of changed files in the check for new tests in unit test mode. This prevents some false positives and should improve the count in certain/rare situations. Prior to this, it just looked for (something)/test(something) in the actual patch... which meant test-patch itself would be cleared. (Ironically, test-patch.sh is now capable of testing itself, so ... yeah. test-ception.)
          • in developer mode when --resetrepo is used, the user output reminds that the code the user chose to run in a destructive mode. (--resetrepo is the default in jenkins mode, obviously)
          • Added code to chmod +x all directories in echo_and_redirect. This should cover every maven run.
          • Cleaned up the eclipse description.
          • Checkstyle now uses the $ {PROJECT_NAME}

            var instead of the hard coded hadoop value, making the code more portable.

          • Reworded some debug output for test subsystems when they are triggered
          • verify_needed_test was missing documentation

          Adding audience / stability comments is a really nice touch.

          Thanks. If you run shelldoc.py (after HADOOP-11845 is applied... ugh) against the code, you'll get a markdown document of the function calls. We're currently using this same functionality in trunk to document hadoop-functions.sh as the 'official' shell API for shell profiles.

          Show
          aw Allen Wittenauer added a comment - - edited -18: add --modulelist and --testlist options to forcibly add modules and test subsystems to a run. there is no way to delete, but that's probably a good thing really. removed the branch restrictions since this apparently won't impact jenkins the check for new tests actually uses the /test/ file path against the list of changed files in the check for new tests in unit test mode. This prevents some false positives and should improve the count in certain/rare situations. Prior to this, it just looked for (something)/test(something) in the actual patch... which meant test-patch itself would be cleared. (Ironically, test-patch.sh is now capable of testing itself, so ... yeah. test-ception.) in developer mode when --resetrepo is used, the user output reminds that the code the user chose to run in a destructive mode. (--resetrepo is the default in jenkins mode, obviously) Added code to chmod +x all directories in echo_and_redirect. This should cover every maven run. Cleaned up the eclipse description. Checkstyle now uses the $ {PROJECT_NAME} var instead of the hard coded hadoop value, making the code more portable. Reworded some debug output for test subsystems when they are triggered verify_needed_test was missing documentation Adding audience / stability comments is a really nice touch. Thanks. If you run shelldoc.py (after HADOOP-11845 is applied... ugh) against the code, you'll get a markdown document of the function calls. We're currently using this same functionality in trunk to document hadoop-functions.sh as the 'official' shell API for shell profiles.
          Hide
          cnauroth Chris Nauroth added a comment -

          +1 from me also after resolution of Sean's comments. (Thanks, Sean!)

          Show
          cnauroth Chris Nauroth added a comment - +1 from me also after resolution of Sean's comments. (Thanks, Sean!)
          Hide
          busbey Sean Busbey added a comment -

          +1 after non-nit issues below. Adding audience / stability comments is a really nice touch.

          +## @description  Some crazy people like Eclipse.  Make sure Maven's eclipse generation
          +## @description  works so they don't freak out.
          

          Please leave out the ableist pejorative.

          +function check_unittests
          +{
          +  verify_needed_test unit
          +
          +  if [[ $? == 0 ]]; then
          +    echo "Existing unit tests do not test patched files. Skipping."
          +    return 0
          +  fi
          
          +  big_console_header "Running unit tests"
          

          Nit: could we make this (and maybe the module choosing) optional so that if I know tests end up covering my changes indirectly I can set a flag to run through things? (Although maybe I missed how I could do this myself)

          This will mostly be useful when I try to merge this set of changes with things over in HBase, since we run through all the unit tests on pre-patch.

          +    if [[ -d "${mypwd}/hadoop-hdfs-project/hadoop-hdfs/target/test/data/dfs" ]]; then
          +      echo "Changing permission on ${mypwd}/hadoop-hdfs-project/hadoop-hdfs/target/test/data/dfs to avoid broken builds"
          +      chmod +x -R "${mypwd}/hadoop-hdfs-project/hadoop-hdfs/target/test/data/dfs"
          +    fi
          

          Nit: so long as we're cleaning things up, could we replace this with a general solution? The root cause of the issue that introduced this change originally was that we need to ensure any directories that exist under a maven module target directory are executable. We know tests can break that one, but there's no reason some other module might break a different one later.

          Show
          busbey Sean Busbey added a comment - +1 after non-nit issues below. Adding audience / stability comments is a really nice touch. +## @description Some crazy people like Eclipse. Make sure Maven's eclipse generation +## @description works so they don't freak out. Please leave out the ableist pejorative. +function check_unittests +{ + verify_needed_test unit + + if [[ $? == 0 ]]; then + echo "Existing unit tests do not test patched files. Skipping." + return 0 + fi + big_console_header "Running unit tests" Nit: could we make this (and maybe the module choosing) optional so that if I know tests end up covering my changes indirectly I can set a flag to run through things? (Although maybe I missed how I could do this myself) This will mostly be useful when I try to merge this set of changes with things over in HBase, since we run through all the unit tests on pre-patch. + if [[ -d "${mypwd}/hadoop-hdfs-project/hadoop-hdfs/target/test/data/dfs" ]]; then + echo "Changing permission on ${mypwd}/hadoop-hdfs-project/hadoop-hdfs/target/test/data/dfs to avoid broken builds" + chmod +x -R "${mypwd}/hadoop-hdfs-project/hadoop-hdfs/target/test/data/dfs" + fi Nit: so long as we're cleaning things up, could we replace this with a general solution? The root cause of the issue that introduced this change originally was that we need to ensure any directories that exist under a maven module target directory are executable. We know tests can break that one, but there's no reason some other module might break a different one later.
          Hide
          busbey Sean Busbey added a comment -

          The jenkins job should be restoring a clean workspace at job start, so we shouldn't need to worry about changing branches back after.

          Show
          busbey Sean Busbey added a comment - The jenkins job should be restoring a clean workspace at job start, so we shouldn't need to worry about changing branches back after.
          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/12726104/HADOOP-11746-17.patch
          against trunk revision 76e7264.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 4 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6115//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6115//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/12726104/HADOOP-11746-17.patch against trunk revision 76e7264. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6115//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6115//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          After sleeping on it, I think it's better to be safe than sorry in case of problems. A few extra lines for extra paranoia...

          -17:

          • During re-exec mode, make sure jenkins reports the console in the "switching" message in case something goes wrong.
          • In reset repo mode, always force checkout to trunk after cleaning the repo area. This might not actually matter in the grand scheme of things, but if something goes haywire in the middle this should provide some level of safety to get us back to a clean slate.
          • Don't switch to any branches that match: branch-0, branch-1, branch-2.[0-7]. It should be noted that currently the code to find a branch won't properly detect minor or micro version branches anyway, so the branch-2 minor version lockout is just in case someone fixes that bug later.

          It should be safe to commit this to branch-2 and have the system come back to trunk for 'real' patches.

          Show
          aw Allen Wittenauer added a comment - After sleeping on it, I think it's better to be safe than sorry in case of problems. A few extra lines for extra paranoia... -17: During re-exec mode, make sure jenkins reports the console in the "switching" message in case something goes wrong. In reset repo mode, always force checkout to trunk after cleaning the repo area. This might not actually matter in the grand scheme of things, but if something goes haywire in the middle this should provide some level of safety to get us back to a clean slate. Don't switch to any branches that match: branch-0, branch-1, branch-2. [0-7] . It should be noted that currently the code to find a branch won't properly detect minor or micro version branches anyway, so the branch-2 minor version lockout is just in case someone fixes that bug later. It should be safe to commit this to branch-2 and have the system come back to trunk for 'real' patches.
          Hide
          aw Allen Wittenauer added a comment - - edited

          (Although, really, that's a problem with the branch detection period unless we backport it to every single branch. So we probably just need to commit this to a reasonable number of branches (branch-1, branch-2, branch-2.7, trunk, ... ? ) and just be aware that someone can upload a patch that may throw things off.)

          Show
          aw Allen Wittenauer added a comment - - edited (Although, really, that's a problem with the branch detection period unless we backport it to every single branch. So we probably just need to commit this to a reasonable number of branches (branch-1, branch-2, branch-2.7, trunk, ... ? ) and just be aware that someone can upload a patch that may throw things off.)
          Hide
          aw Allen Wittenauer added a comment - - edited

          A few things:

          a) It'd be great if HADOOP-11778 was committed as well. This updates the maven checkstyle plugin so that it doesn't NPE on our code base in certain conditions, at least on trunk. It doesn't break the plugin, but it does mean the results may not be 100% accurate.

          b) We need to be aware that shellcheck isn't installed on the jenkins boxes yet. This means the shellcheck tests won't execute until that's rectified. (That plugin shouldn't error, however, since the code does try to see if it is installed first.)

          c) We'll likely need to commit to branch-2 anyway if we want the patch branch detection code to work. I don't know how jenkins actually uses test-patch.sh, but this code does not switch the branch back to trunk after it runs on another branch. This might very well be a bug, now that I think about it more.

          Show
          aw Allen Wittenauer added a comment - - edited A few things: a) It'd be great if HADOOP-11778 was committed as well. This updates the maven checkstyle plugin so that it doesn't NPE on our code base in certain conditions, at least on trunk. It doesn't break the plugin, but it does mean the results may not be 100% accurate. b) We need to be aware that shellcheck isn't installed on the jenkins boxes yet. This means the shellcheck tests won't execute until that's rectified. (That plugin shouldn't error, however, since the code does try to see if it is installed first.) c) We'll likely need to commit to branch-2 anyway if we want the patch branch detection code to work. I don't know how jenkins actually uses test-patch.sh, but this code does not switch the branch back to trunk after it runs on another branch. This might very well be a bug, now that I think about it more.
          Hide
          cnauroth Chris Nauroth added a comment -

          +1 for patch v16. All of my feedback has been addressed either by code changes or explaining parts of the logic that I misunderstood.

          Since this impacts everyone's day-to-day work, can we please hold off committing until Friday, 4/17, just in case there are other opinions? I'll send notice now to the dev lists.

          I have just one more procedural question. I see this is flagged as an incompatible change. While that's technically true, I don't think compatibility concerns apply to our internal dev tooling. If it's flagged as incompatible, then it may cause confusion for Hadoop end users who think they need to do something in their deployments to react to this incompatibility. What do you think?

          Show
          cnauroth Chris Nauroth added a comment - +1 for patch v16. All of my feedback has been addressed either by code changes or explaining parts of the logic that I misunderstood. Since this impacts everyone's day-to-day work, can we please hold off committing until Friday, 4/17, just in case there are other opinions? I'll send notice now to the dev lists. I have just one more procedural question. I see this is flagged as an incompatible change. While that's technically true, I don't think compatibility concerns apply to our internal dev tooling. If it's flagged as incompatible, then it may cause confusion for Hadoop end users who think they need to do something in their deployments to react to this incompatibility. What do you think?
          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/12725807/HADOOP-11746-16.patch
          against trunk revision 1b89a3e.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 4 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6111//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6111//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/12725807/HADOOP-11746-16.patch against trunk revision 1b89a3e. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6111//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6111//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          Oh, forgot:

          Are you planning to clean up smart-apply-patch.sh in scope of this jira, or will that happen elsewhere?

          I was planning on ignoring smart-apply-patch for the time being. Raymie Stata's patch in HADOOP-11781 fixed the dangerous problems. Plus the HBase people are putting the pressure on for me to start work on HBASE-13231, especially since stack is going around telling people publicly the new shell code will be a feature in 2.0...

          Show
          aw Allen Wittenauer added a comment - Oh, forgot: Are you planning to clean up smart-apply-patch.sh in scope of this jira, or will that happen elsewhere? I was planning on ignoring smart-apply-patch for the time being. Raymie Stata 's patch in HADOOP-11781 fixed the dangerous problems. Plus the HBase people are putting the pressure on for me to start work on HBASE-13231 , especially since stack is going around telling people publicly the new shell code will be a feature in 2.0...
          Hide
          aw Allen Wittenauer added a comment -

          -16:

          • Fixed, etc through all of the Chris Nauroth's review comments. Much thanks!

          Some notes:

          checkstyle.sh

          2. Do you think it's worthwhile to move the Python code into its own .py file?

          I've been debating this myself. Two advantages of doing it this way:

          • the whole plug-in is self contained.
          • the dev-support patch detection is a lot easier

          shellcheck.sh

          1. shellcheck_private_findbash: This looks for files in bin and sbin. Do we also need libexec, which is currently used in hadoop-kms and hadoop-hdfs-httpfs?

          Great catch. I also added shellprofile.d while I was there!

          whitespace.sh

          Ugh. Good point. That should be [[:blank:]] not [[:space:]]. Easy fix. This also means this works in non-C locales a bit better.

          testpatch.sh

          2. determine_needed_tests: I believe this would not identify tests for a patch that contained only changes in test resources (not Java test code). Examples of this include testConf.xml and editsStored and editsStored.xml.

          Relevant code:

              elif [[ ${i} =~ \.c$
          ...
                || ${i} =~ src/test
          ...
                 ]]; then
                 hadoop_debug "tests/native: ${i}"
                 add_test javac
                 add_test unit
          

          It should. If any file has src/test in its path, it will trigger the javac and unit tests. It's really not "tests/native", so maybe that should get renamed to something else I guess. (This is counter to tests/java which also triggers javadoc tests)

          4. output_to_console: Today I learned that your secret talent is ASCII elephant art.

          I don't know what you are talking about. Maybe your patch download was bad?

          5. There are shellcheck warnings on lines 1158, 1573, 1732, 1751, and 1995.

          Fixed or disabled all of these except two, which are a) false positives and b) extremely hard to escape because they are actually awk commands in a multiline pipe.

          6. The old test-patch.sh output always included a hyperlink to the patch file that it ran. Can we please add that? I always found that helpful for knowing exactly which patch it was reporting on, in case multiple revisions got uploaded quickly.

          It actually does this when the patch provided is from either a JIRA or a URL. This is consistent with the old code.

          FWIW, I've uploaded the same patch into HADOOP-11820 and ran it in jenkins mode. You'll see the shellcheck #'s and the patch URL there as well. Check out that execution time.

          Show
          aw Allen Wittenauer added a comment - -16: Fixed, etc through all of the Chris Nauroth 's review comments. Much thanks! Some notes: checkstyle.sh 2. Do you think it's worthwhile to move the Python code into its own .py file? I've been debating this myself. Two advantages of doing it this way: the whole plug-in is self contained. the dev-support patch detection is a lot easier shellcheck.sh 1. shellcheck_private_findbash : This looks for files in bin and sbin. Do we also need libexec, which is currently used in hadoop-kms and hadoop-hdfs-httpfs? Great catch. I also added shellprofile.d while I was there! whitespace.sh Ugh. Good point. That should be [ [:blank:] ] not [ [:space:] ]. Easy fix. This also means this works in non-C locales a bit better. testpatch.sh 2. determine_needed_tests : I believe this would not identify tests for a patch that contained only changes in test resources (not Java test code). Examples of this include testConf.xml and editsStored and editsStored.xml. Relevant code: elif [[ ${i} =~ \.c$ ... || ${i} =~ src/test ... ]]; then hadoop_debug "tests/ native : ${i}" add_test javac add_test unit It should. If any file has src/test in its path, it will trigger the javac and unit tests. It's really not "tests/native", so maybe that should get renamed to something else I guess. (This is counter to tests/java which also triggers javadoc tests) 4. output_to_console : Today I learned that your secret talent is ASCII elephant art. I don't know what you are talking about. Maybe your patch download was bad? 5. There are shellcheck warnings on lines 1158, 1573, 1732, 1751, and 1995. Fixed or disabled all of these except two, which are a) false positives and b) extremely hard to escape because they are actually awk commands in a multiline pipe. 6. The old test-patch.sh output always included a hyperlink to the patch file that it ran. Can we please add that? I always found that helpful for knowing exactly which patch it was reporting on, in case multiple revisions got uploaded quickly. It actually does this when the patch provided is from either a JIRA or a URL. This is consistent with the old code. FWIW, I've uploaded the same patch into HADOOP-11820 and ran it in jenkins mode. You'll see the shellcheck #'s and the patch URL there as well. Check out that execution time.
          Hide
          cnauroth Chris Nauroth added a comment -

          Thank you for the patch, Allen. The new functionality looks great! Here are a few comments.

          checkstyle.sh

          1. checkstyle_postapply: Please correct the indentation on this line.
             cp -p "${BASEDIR}/target/checkstyle-result.xml" "${PATCH_DIR}/checkstyle-result-patch.xml"
            
          2. Do you think it's worthwhile to move the Python code into its own .py file?

          shellcheck.sh

          1. shellcheck_private_findbash: This looks for files in bin and sbin. Do we also need libexec, which is currently used in hadoop-kms and hadoop-hdfs-httpfs?
          2. There are shellcheck warnings on lines 49 and 70. I see shellcheck.sh caught them in the last run, so that's cool.

          whitespace.sh

          1. From some quick testing of the grep, it's catching trailing spaces, but not trailing tabs.

          test-patch.sh

          1. precheck_without_patch and check_site: The mvn site command that is echoed is slightly different from what is actually run. The arguments are in a different order. I wonder if it would be helpful to have a echo_and_exec helper function to call everywhere that we do this kind of thing.
          2. determine_needed_tests: I believe this would not identify tests for a patch that contained only changes in test resources (not Java test code). Examples of this include testConf.xml and editsStored and editsStored.xml.
          3. check_unittests: The echo of the mvn command on line 1736 does not include the redirection of output to test_logfile as the actual executed command does.
          4. output_to_console: Today I learned that your secret talent is ASCII elephant art.
          5. There are shellcheck warnings on lines 1158, 1573, 1732, 1751, and 1995.
          6. The old test-patch.sh output always included a hyperlink to the patch file that it ran. Can we please add that? I always found that helpful for knowing exactly which patch it was reporting on, in case multiple revisions got uploaded quickly.

          Are you planning to clean up smart-apply-patch.sh in scope of this jira, or will that happen elsewhere?

          Show
          cnauroth Chris Nauroth added a comment - Thank you for the patch, Allen. The new functionality looks great! Here are a few comments. checkstyle.sh checkstyle_postapply : Please correct the indentation on this line. cp -p "${BASEDIR}/target/checkstyle-result.xml" "${PATCH_DIR}/checkstyle-result-patch.xml" Do you think it's worthwhile to move the Python code into its own .py file? shellcheck.sh shellcheck_private_findbash : This looks for files in bin and sbin. Do we also need libexec, which is currently used in hadoop-kms and hadoop-hdfs-httpfs? There are shellcheck warnings on lines 49 and 70. I see shellcheck.sh caught them in the last run, so that's cool. whitespace.sh From some quick testing of the grep, it's catching trailing spaces, but not trailing tabs. test-patch.sh precheck_without_patch and check_site : The mvn site command that is echoed is slightly different from what is actually run. The arguments are in a different order. I wonder if it would be helpful to have a echo_and_exec helper function to call everywhere that we do this kind of thing. determine_needed_tests : I believe this would not identify tests for a patch that contained only changes in test resources (not Java test code). Examples of this include testConf.xml and editsStored and editsStored.xml. check_unittests : The echo of the mvn command on line 1736 does not include the redirection of output to test_logfile as the actual executed command does. output_to_console : Today I learned that your secret talent is ASCII elephant art. There are shellcheck warnings on lines 1158, 1573, 1732, 1751, and 1995. The old test-patch.sh output always included a hyperlink to the patch file that it ran. Can we please add that? I always found that helpful for knowing exactly which patch it was reporting on, in case multiple revisions got uploaded quickly. Are you planning to clean up smart-apply-patch.sh in scope of this jira, or will that happen elsewhere?
          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/12724628/HADOOP-11746-15.patch
          against trunk revision 7660da9.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 4 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6093//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6093//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/12724628/HADOOP-11746-15.patch against trunk revision 7660da9. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6093//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6093//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          -15:

          • go back to git pull rebase
          Show
          aw Allen Wittenauer added a comment - -15: go back to git pull rebase
          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/12724454/HADOOP-11746-14.patch
          against trunk revision af9d4fe.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

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

          -1 javac. The applied patch generated 1201 javac compiler warnings (more than the trunk's current 1163 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6092//testReport/
          Javac warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/6092//artifact/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6092//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/12724454/HADOOP-11746-14.patch against trunk revision af9d4fe. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 new or modified test files. -1 javac . The applied patch generated 1201 javac compiler warnings (more than the trunk's current 1163 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6092//testReport/ Javac warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/6092//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6092//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 00s dev-support patch detected.
          0 pre-patch 0m 00s Pre-patch trunk compilation is healthy.
          -1 @author 0m 00s The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.
          +1 whitespace 0m 00s The patch has no lines that end in whitespace.
          +1 release audit 0m 09s The applied patch does not increase the total number of release audit warnings.
          -1 shellcheck 0m 03s The applied patch generated 7 new shellcheck issues (total was 165, now 28).
              0m 14s  



          Subsystem Report/Notes
          Optional Tests shellcheck
          git revision trunk / f4b3fc5
          shellcheck https://builds.apache.org/job/PreCommit-HADOOP-Build/fake/artifact/patchprocess/diffpatchshellcheck.txt
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/fake//console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 00s dev-support patch detected. 0 pre-patch 0m 00s Pre-patch trunk compilation is healthy. -1 @author 0m 00s The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 whitespace 0m 00s The patch has no lines that end in whitespace. +1 release audit 0m 09s The applied patch does not increase the total number of release audit warnings. -1 shellcheck 0m 03s The applied patch generated 7 new shellcheck issues (total was 165, now 28).     0m 14s   Subsystem Report/Notes Optional Tests shellcheck git revision trunk / f4b3fc5 shellcheck https://builds.apache.org/job/PreCommit-HADOOP-Build/fake/artifact/patchprocess/diffpatchshellcheck.txt Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/fake//console This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          A patch to test-patch or smart-apply-patch has been detected.
          Re-executing against the patched versions to perform further tests.

          Show
          hadoopqa Hadoop QA added a comment - A patch to test-patch or smart-apply-patch has been detected. Re-executing against the patched versions to perform further tests.
          Hide
          aw Allen Wittenauer added a comment -

          -14:

          • add --offline mode to prevent connections to the internet. extremely useful on planes and trains.
          • changed the unit test output so that one can see runtimes per module. extremely useful to figure out which set of tests need some love
          • changed how timers work so that there is now an timer for the entire run
          • after a lot of testing with bigger patches, increased the minute marks to be 3 digits since some tests run for hours.
          • lots of other, more minor output tweaking, esp after doing a better jenkins simulation
          • don't print out the reexec command line because of security issues
          Show
          aw Allen Wittenauer added a comment - -14: add --offline mode to prevent connections to the internet. extremely useful on planes and trains. changed the unit test output so that one can see runtimes per module . extremely useful to figure out which set of tests need some love changed how timers work so that there is now an timer for the entire run after a lot of testing with bigger patches, increased the minute marks to be 3 digits since some tests run for hours. lots of other, more minor output tweaking, esp after doing a better jenkins simulation don't print out the reexec command line because of security issues
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 00m 00s dev-support patch detected.
          0 pre-patch 00m 00s Pre-patch trunk compilation is healthy.
          -1 @author 00m 00s The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.
          +1 whitespace 00m 00s The patch has no lines that end in whitespace.
          +1 release audit 00m 09s The applied patch does not increase the total number of release audit warnings.
          -1 shellcheck 00m 03s The applied patch generated 7 new shellcheck issues (total was 165, now 28).
              00m 14s  



          Subsystem Report/Notes
          Optional Tests shellcheck
          git revision f4b3fc5 / trunk
          shellcheck https://builds.apache.org/job/PreCommit-HADOOP-Build/fake//artifact/patchprocessdiffpatchshellcheck.txt
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/fake//artifact/patchprocessconsole

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 00m 00s dev-support patch detected. 0 pre-patch 00m 00s Pre-patch trunk compilation is healthy. -1 @author 00m 00s The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 whitespace 00m 00s The patch has no lines that end in whitespace. +1 release audit 00m 09s The applied patch does not increase the total number of release audit warnings. -1 shellcheck 00m 03s The applied patch generated 7 new shellcheck issues (total was 165, now 28).     00m 14s   Subsystem Report/Notes Optional Tests shellcheck git revision f4b3fc5 / trunk shellcheck https://builds.apache.org/job/PreCommit-HADOOP-Build/fake//artifact/patchprocessdiffpatchshellcheck.txt Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/fake//artifact/patchprocessconsole This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          A patch to test-patch or smart-apply-patch has been detected.
          Re-executing against the patched versions to perform further tests.

          Show
          hadoopqa Hadoop QA added a comment - A patch to test-patch or smart-apply-patch has been detected. Re-executing against the patched versions to perform further tests.
          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/12724277/HADOOP-11746-13.patch
          against trunk revision 1885141.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 4 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6087//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6087//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/12724277/HADOOP-11746-13.patch against trunk revision 1885141. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6087//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6087//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment - - edited

          -13:

          • fix an issue where a git ref may be in the future vs. the copy on disk. We no longer do a pull --rebase, but a fetch + checkout --force in jenkins mode.
          • shellcheck support beefed up, including now failing on new errors, not if the total count was higher. Also smarter about picking up on patches that patch shell code that are not in the normal places
          • cosmetic fixes for the test table
          • renamed populate_unit_table to populate_test_table to better reflect what it is
          • findbugs moved into its own optional test instead of being tied to javac so that pom.xml, native code, etc, won't trigger a run. (This does mean that testing findbugs itself will get missed now... but that's a very rare occurrence...)



          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 00m 00s dev-support patch detected.
          0 pre-patch 00m 00s Pre-patch trunk compilation is healthy.
          -1 @author 00m 00s The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.
          +1 whitespace 00m 00s The patch has no lines that end in whitespace.
          +1 release audit 00m 08s The applied patch does not increase the total number of release audit warnings.
          -1 shellcheck 00m 03s The applied patch generated 6 new shellcheck issues (total was 165, now 27).
          Show
          aw Allen Wittenauer added a comment - - edited -13: fix an issue where a git ref may be in the future vs. the copy on disk. We no longer do a pull --rebase, but a fetch + checkout --force in jenkins mode. shellcheck support beefed up, including now failing on new errors, not if the total count was higher. Also smarter about picking up on patches that patch shell code that are not in the normal places cosmetic fixes for the test table renamed populate_unit_table to populate_test_table to better reflect what it is findbugs moved into its own optional test instead of being tied to javac so that pom.xml, native code, etc, won't trigger a run. (This does mean that testing findbugs itself will get missed now... but that's a very rare occurrence...) -1 overall Vote Subsystem Runtime Comment 0 reexec 00m 00s dev-support patch detected. 0 pre-patch 00m 00s Pre-patch trunk compilation is healthy. -1 @author 00m 00s The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 whitespace 00m 00s The patch has no lines that end in whitespace. +1 release audit 00m 08s The applied patch does not increase the total number of release audit warnings. -1 shellcheck 00m 03s The applied patch generated 6 new shellcheck issues (total was 165, now 27).
          Hide
          aw Allen Wittenauer added a comment -

          Console summary for previous Java example:

          
          -1 overall
          
          | Vote |           Subsystem |  Runtime  | Comment
          |   0  |          pre-patch  |  08m 44s  | Pre-patch trunk compilation is 
          |      |                     |           | healthy.
          |  +1  |            @author  |  00m 00s  | The patch does not contain any 
          |      |                     |           | @author tags.
          |  -1  |     tests included  |  00m 00s  | 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  |         whitespace  |  00m 00s  | The patch has 1 lines that end in 
          |      |                     |           | whitespace.
          |  +1  |              javac  |  04m 09s  | There were no new javac warning 
          |      |                     |           | messages.
          |  +1  |            javadoc  |  05m 41s  | There were no new javadoc warning 
          |      |                     |           | messages.
          |  +1  |      release audit  |  00m 11s  | The applied patch does not increase 
          |      |                     |           | the total number of release audit
          |      |                     |           | warnings.
          |  +1  |         checkstyle  |  03m 22s  | There were no new checkstyle issues. 
          |  +1  |            install  |  01m 00s  | mvn install still works. 
          |  +1  |    eclipse:eclipse  |  00m 22s  | The patch built with 
          |      |                     |           | eclipse:eclipse.
          |  -1  |           findbugs  |  00m 51s  | The patch appears to introduce 2 new 
          |      |                     |           | Findbugs (version 3.0.1) warnings.
          |  -1  |         core tests  |  01m 20s  | Tests failed in 
          |      |                     |           | hadoop-yarn-project/hadoop-yarn/hadoop
          |      |                     |           | -yarn-common.
          
          
                       Reason | Tests
                    FindBugs  |  FindBugs 
                              |  Unread public/protected field:At Log4jWarningErrorMetricsAppender.java:[line 44] 
                    FindBugs  |  FindBugs 
                              |  Unread public/protected field:At Log4jWarningErrorMetricsAppender.java:[line 45] 
           Failed unit tests  |  Failed unit tests 
                              |  hadoop.yarn.util.TestLog4jWarningErrorMetricsAppender 
          
          
          || Subsystem || Report/Notes ||
          | Patch URL | http://issues.apache.org/jira/secure/attachment/12723637/MAPREDUCE-6301-002.patch |
          | Optional Tests | javadoc javac unit checkstyle |
          | git revision | f4b3fc5 / trunk |
          | whitespace | /tmp/hadoop-test-patch/75950/whitespace.txt |
          | Findbugs warnings | /tmp/hadoop-test-patch/75950/newPatchFindbugsWarningshadoop-yarn-common.html |
          | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common test log | /tmp/hadoop-test-patch/75950/testrun_hadoop-yarn-common.txt |
          | Test Results | /testReport/ |
          
          Show
          aw Allen Wittenauer added a comment - Console summary for previous Java example: -1 overall | Vote | Subsystem | Runtime | Comment | 0 | pre-patch | 08m 44s | Pre-patch trunk compilation is | | | | healthy. | +1 | @author | 00m 00s | The patch does not contain any | | | | @author tags. | -1 | tests included | 00m 00s | 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 | whitespace | 00m 00s | The patch has 1 lines that end in | | | | whitespace. | +1 | javac | 04m 09s | There were no new javac warning | | | | messages. | +1 | javadoc | 05m 41s | There were no new javadoc warning | | | | messages. | +1 | release audit | 00m 11s | The applied patch does not increase | | | | the total number of release audit | | | | warnings. | +1 | checkstyle | 03m 22s | There were no new checkstyle issues. | +1 | install | 01m 00s | mvn install still works. | +1 | eclipse:eclipse | 00m 22s | The patch built with | | | | eclipse:eclipse. | -1 | findbugs | 00m 51s | The patch appears to introduce 2 new | | | | Findbugs (version 3.0.1) warnings. | -1 | core tests | 01m 20s | Tests failed in | | | | hadoop-yarn-project/hadoop-yarn/hadoop | | | | -yarn-common. Reason | Tests FindBugs | FindBugs | Unread public/protected field:At Log4jWarningErrorMetricsAppender.java:[line 44] FindBugs | FindBugs | Unread public/protected field:At Log4jWarningErrorMetricsAppender.java:[line 45] Failed unit tests | Failed unit tests | hadoop.yarn.util.TestLog4jWarningErrorMetricsAppender || Subsystem || Report/Notes || | Patch URL | http://issues.apache.org/jira/secure/attachment/12723637/MAPREDUCE-6301-002.patch | | Optional Tests | javadoc javac unit checkstyle | | git revision | f4b3fc5 / trunk | | whitespace | /tmp/hadoop-test-patch/75950/whitespace.txt | | Findbugs warnings | /tmp/hadoop-test-patch/75950/newPatchFindbugsWarningshadoop-yarn-common.html | | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common test log | /tmp/hadoop-test-patch/75950/testrun_hadoop-yarn-common.txt | | Test Results | /testReport/ |
          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/12724125/HADOOP-11746-12.patch
          against trunk revision dc0282d.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 4 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6084//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6084//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/12724125/HADOOP-11746-12.patch against trunk revision dc0282d. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6084//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6084//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          For comparison, I ran MAPREDUCE-6301 through it which has some known issues just so everyone can see what a Java failure looks like. It downloaded the current patch (MR-6301-002) and gave this output:



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 08m 44s Pre-patch trunk compilation is healthy.
          +1 @author 00m 00s The patch does not contain any @author tags.
          -1 tests included 00m 00s 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 whitespace 00m 00s The patch has 1 lines that end in whitespace.
          +1 javac 04m 09s There were no new javac warning messages.
          +1 javadoc 05m 41s There were no new javadoc warning messages.
          +1 release audit 00m 11s The applied patch does not increase the total number of release audit warnings.
          +1 checkstyle 03m 22s There were no new checkstyle issues.
          +1 install 01m 00s mvn install still works.
          +1 eclipse:eclipse 00m 22s The patch built with eclipse:eclipse.
          -1 findbugs 00m 51s The patch appears to introduce 2 new Findbugs (version 3.0.1) warnings.
          -1 core tests 01m 20s Tests failed in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common.



          Reason Tests
          FindBugs FindBugs
            Unread public/protected field:At Log4jWarningErrorMetricsAppender.java:[line 44]
          FindBugs FindBugs
            Unread public/protected field:At Log4jWarningErrorMetricsAppender.java:[line 45]
          Failed unit tests Failed unit tests
            hadoop.yarn.util.TestLog4jWarningErrorMetricsAppender



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12723637/MAPREDUCE-6301-002.patch
          Optional Tests javadoc javac unit checkstyle
          git revision f4b3fc5 / trunk
          whitespace /artifact/patchprocess/whitespace.txt
          Findbugs warnings /artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-common.html
          hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common test log /artifact/patchprocess/testrun_hadoop-yarn-common.txt
          Test Results /testReport/
          Console output /artifact/patchprocess/console

          This message was automatically generated.

          Show
          aw Allen Wittenauer added a comment - For comparison, I ran MAPREDUCE-6301 through it which has some known issues just so everyone can see what a Java failure looks like. It downloaded the current patch (MR-6301-002) and gave this output: -1 overall Vote Subsystem Runtime Comment 0 pre-patch 08m 44s Pre-patch trunk compilation is healthy. +1 @author 00m 00s The patch does not contain any @author tags. -1 tests included 00m 00s 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 whitespace 00m 00s The patch has 1 lines that end in whitespace. +1 javac 04m 09s There were no new javac warning messages. +1 javadoc 05m 41s There were no new javadoc warning messages. +1 release audit 00m 11s The applied patch does not increase the total number of release audit warnings. +1 checkstyle 03m 22s There were no new checkstyle issues. +1 install 01m 00s mvn install still works. +1 eclipse:eclipse 00m 22s The patch built with eclipse:eclipse. -1 findbugs 00m 51s The patch appears to introduce 2 new Findbugs (version 3.0.1) warnings. -1 core tests 01m 20s Tests failed in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common. Reason Tests FindBugs FindBugs   Unread public/protected field:At Log4jWarningErrorMetricsAppender.java: [line 44] FindBugs FindBugs   Unread public/protected field:At Log4jWarningErrorMetricsAppender.java: [line 45] Failed unit tests Failed unit tests   hadoop.yarn.util.TestLog4jWarningErrorMetricsAppender Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12723637/MAPREDUCE-6301-002.patch Optional Tests javadoc javac unit checkstyle git revision f4b3fc5 / trunk whitespace /artifact/patchprocess/whitespace.txt Findbugs warnings /artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-common.html hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common test log /artifact/patchprocess/testrun_hadoop-yarn-common.txt Test Results /testReport/ Console output /artifact/patchprocess/console This message was automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          If I rename the patch file to be HADOOP-11746-12.git5449adc.patch, it provides this output in the JIRA message. Note how it gives the 5449adc reference instead of trunk above.



          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 00m 00s dev-support patch detected.
          0 pre-patch 00m 00s Pre-patch 5449adc compilation is healthy.
          -1 @author 00m 00s The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.
          +1 whitespace 00m 00s The patch has no lines that end in whitespace.
          +1 release audit 00m 09s The applied patch does not increase the total number of release audit warnings.
          +1 shellcheck 00m 02s There were no new shellcheck issues.



          Subsystem Report/Notes
          Optional Tests shellcheck
          git revision 5449adc / 5449adc
          Console output /artifact/patchprocess/console
          Show
          aw Allen Wittenauer added a comment - If I rename the patch file to be HADOOP-11746 -12.git5449adc.patch, it provides this output in the JIRA message. Note how it gives the 5449adc reference instead of trunk above. -1 overall Vote Subsystem Runtime Comment 0 reexec 00m 00s dev-support patch detected. 0 pre-patch 00m 00s Pre-patch 5449adc compilation is healthy. -1 @author 00m 00s The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 whitespace 00m 00s The patch has no lines that end in whitespace. +1 release audit 00m 09s The applied patch does not increase the total number of release audit warnings. +1 shellcheck 00m 02s There were no new shellcheck issues. Subsystem Report/Notes Optional Tests shellcheck git revision 5449adc / 5449adc Console output /artifact/patchprocess/console
          Hide
          aw Allen Wittenauer added a comment - - edited

          -12:

          • add --resetrepo to simulate jenkins git repo nuker in developer mode
          • don't send a jira message and abort early if the URL doesn't end in .patch
          • Allow git(8 chars) or git(41 chars) as branch names for patch testing
          • make it explicit in the report output if we were in branch or git ref mode
          • fix some issues with checkstyle
          • when using --resetrepo mode, shellcheck would erroneously flag .orig and .rej files from previous patches since those aren't cleared by git due to .gitignore
          • fix a few shellcheck warnings I had missed. (shellcheck is still mostly ignoring dev-support. should probably fix that.)
          • findbugs now gives a summary report on the console/jira output rather than forcing folks to look at the full findbugs output (altho that is still listed too!)
          • set the TIMER default at startup to be current time rather 0 to prevent incredibly bogus, decades long runtimes in case of early aborts
          • re-order the flags in the options parser
          • fix an issue with the test heuristics so that external plugins aren't prematurely ignored

          Testing itself, it says:



          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 00m 00s dev-support patch detected.
          0 pre-patch 00m 00s Pre-patch trunk compilation is healthy.
          -1 @author 00m 00s The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.
          +1 whitespace 00m 00s The patch has no lines that end in whitespace.
          +1 release audit 00m 09s The applied patch does not increase the total number of release audit warnings.
          +1 shellcheck 00m 02s There were no new shellcheck issues.



          Subsystem Report/Notes
          Optional Tests shellcheck
          git revision f4b3fc5 / trunk
          Console output /artifact/patchprocess/console

          This message was automatically generated.

          Show
          aw Allen Wittenauer added a comment - - edited -12: add --resetrepo to simulate jenkins git repo nuker in developer mode don't send a jira message and abort early if the URL doesn't end in .patch Allow git(8 chars) or git(41 chars) as branch names for patch testing make it explicit in the report output if we were in branch or git ref mode fix some issues with checkstyle when using --resetrepo mode, shellcheck would erroneously flag .orig and .rej files from previous patches since those aren't cleared by git due to .gitignore fix a few shellcheck warnings I had missed. (shellcheck is still mostly ignoring dev-support. should probably fix that.) findbugs now gives a summary report on the console/jira output rather than forcing folks to look at the full findbugs output (altho that is still listed too!) set the TIMER default at startup to be current time rather 0 to prevent incredibly bogus, decades long runtimes in case of early aborts re-order the flags in the options parser fix an issue with the test heuristics so that external plugins aren't prematurely ignored Testing itself, it says: -1 overall Vote Subsystem Runtime Comment 0 reexec 00m 00s dev-support patch detected. 0 pre-patch 00m 00s Pre-patch trunk compilation is healthy. -1 @author 00m 00s The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 whitespace 00m 00s The patch has no lines that end in whitespace. +1 release audit 00m 09s The applied patch does not increase the total number of release audit warnings. +1 shellcheck 00m 02s There were no new shellcheck issues. Subsystem Report/Notes Optional Tests shellcheck git revision f4b3fc5 / trunk Console output /artifact/patchprocess/console This message was automatically generated.
          Hide
          owen.omalley Owen O'Malley added a comment -

          This looks like a nice improvement.

          It would be great if we could name patch files like git####.patch and have it apply to the git hash ####. That would let you upload patches for branches without worrying about conflicting changes.

          Show
          owen.omalley Owen O'Malley added a comment - This looks like a nice improvement. It would be great if we could name patch files like git####.patch and have it apply to the git hash ####. That would let you upload patches for branches without worrying about conflicting changes.
          Hide
          aw Allen Wittenauer added a comment -

          Another potential improvement on top of that would be to skip runs if the patch file name contains a branch name, because we only do pre-commit for trunk currently.

          This code actually can do pre-commit runs for non-trunk based upon the patch name...

          Show
          aw Allen Wittenauer added a comment - Another potential improvement on top of that would be to skip runs if the patch file name contains a branch name, because we only do pre-commit for trunk currently. This code actually can do pre-commit runs for non-trunk based upon the patch name...
          Hide
          cnauroth Chris Nauroth added a comment -

          As per discussion on common-dev, we'd like to be able to exit early from test-patch runs against an attachment if the file name doesn't match *.patch. This would cut down on spam from Jenkins trying to run test-patch on other kinds of attachments, like screenshots and design docs.

          Another potential improvement on top of that would be to skip runs if the patch file name contains a branch name, because we only do pre-commit for trunk currently.

          Just as a reminder, naming conventions for patch files are documented here:

          https://wiki.apache.org/hadoop/HowToContribute#Naming_your_patch

          Show
          cnauroth Chris Nauroth added a comment - As per discussion on common-dev, we'd like to be able to exit early from test-patch runs against an attachment if the file name doesn't match *.patch. This would cut down on spam from Jenkins trying to run test-patch on other kinds of attachments, like screenshots and design docs. Another potential improvement on top of that would be to skip runs if the patch file name contains a branch name, because we only do pre-commit for trunk currently. Just as a reminder, naming conventions for patch files are documented here: https://wiki.apache.org/hadoop/HowToContribute#Naming_your_patch
          Hide
          aw Allen Wittenauer added a comment -

          Giridharan Kesavan, how do we test this on a jenkins instance?

          Show
          aw Allen Wittenauer added a comment - Giridharan Kesavan , how do we test this on a jenkins instance?
          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/12723822/HADOOP-11746-11.patch
          against trunk revision 4be648b.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 4 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6073//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6073//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/12723822/HADOOP-11746-11.patch against trunk revision 4be648b. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6073//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6073//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          Testing this patch takes less than a minute, BTW. Huge improvement over the 20+ mins it takes to run tests that don't make a bit of difference. It pulls in the shellcheck tests and completely skips the java-related tests. Testing other patches seems to indicate the heuristics do more false positives (more tests than needed) but I haven't seen a situation with false negatives (missing tests that should really be run) yet. They are likely there, but we'll see. The system does report what tests it ran so we should be able to watch for that.

          Show
          aw Allen Wittenauer added a comment - Testing this patch takes less than a minute, BTW. Huge improvement over the 20+ mins it takes to run tests that don't make a bit of difference. It pulls in the shellcheck tests and completely skips the java-related tests. Testing other patches seems to indicate the heuristics do more false positives (more tests than needed) but I haven't seen a situation with false negatives (missing tests that should really be run) yet. They are likely there, but we'll see. The system does report what tests it ran so we should be able to watch for that.
          Hide
          aw Allen Wittenauer added a comment -

          -11:

          • clean up the test selector API so that plugins could easily use it. so now checkstyle and (sometimes) shellcheck only kick in when necessary
          • added mvn install check to javac or javadoc test check. suspect it really only needs javadoc, but let's be conservative for now
          • fixed the regexes on the test selector
          • added cmakelists.txt as a trigger for the native code checks
          Show
          aw Allen Wittenauer added a comment - -11: clean up the test selector API so that plugins could easily use it. so now checkstyle and (sometimes) shellcheck only kick in when necessary added mvn install check to javac or javadoc test check. suspect it really only needs javadoc, but let's be conservative for now fixed the regexes on the test selector added cmakelists.txt as a trigger for the native code checks
          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/12723763/HADOOP-11746-10.patch
          against trunk revision bd77a7c.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 4 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6071//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6071//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/12723763/HADOOP-11746-10.patch against trunk revision bd77a7c. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6071//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6071//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          -10:

          • fix the rat error
          • add better heuristics to determine the minimal amount of long running tests to execute: significantly faster run times for certain classes of patches
          • add a site compilation test; doc patches actually get tested
          • fix a few bugs in the checkstyle plugin
          • fix a few bugs in the whitespace plugin
          Show
          aw Allen Wittenauer added a comment - -10: fix the rat error add better heuristics to determine the minimal amount of long running tests to execute: significantly faster run times for certain classes of patches add a site compilation test; doc patches actually get tested fix a few bugs in the checkstyle plugin fix a few bugs in the whitespace plugin
          Hide
          aw Allen Wittenauer added a comment -

          The rat error is real, the rest are obviously bogus. Will fix and post an update tomorrow.

          Show
          aw Allen Wittenauer added a comment - The rat error is real, the rest are obviously bogus. Will fix and post an update tomorrow.
          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/12723460/HADOOP-11746-09.patch
          against trunk revision 3fb5abf.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

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

          -1 javac. The applied patch generated 1148 javac compiler warnings (more than the trunk's current 208 warnings).

          -1 javadoc. The javadoc tool appears to have generated 43 warning messages.
          See https://builds.apache.org/job/PreCommit-HADOOP-Build/6069//artifact/patchprocess/diffJavadocWarnings.txt for details.

          +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 generated 1 release audit warnings.

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

          org.apache.hadoop.hdfs.TestDFSOutputStream

          The following test timeouts occurred in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.server.balancer.TestBalancerWithMultipleNameNodes

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6069//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/6069//artifact/patchprocess/patchReleaseAuditProblems.txt
          Javac warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/6069//artifact/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6069//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/12723460/HADOOP-11746-09.patch against trunk revision 3fb5abf. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 new or modified test files. -1 javac . The applied patch generated 1148 javac compiler warnings (more than the trunk's current 208 warnings). -1 javadoc . The javadoc tool appears to have generated 43 warning messages. See https://builds.apache.org/job/PreCommit-HADOOP-Build/6069//artifact/patchprocess/diffJavadocWarnings.txt for details. +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 generated 1 release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDFSOutputStream The following test timeouts occurred in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.balancer.TestBalancerWithMultipleNameNodes Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6069//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/6069//artifact/patchprocess/patchReleaseAuditProblems.txt Javac warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/6069//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6069//console This message is automatically generated.
          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/12723454/HADOOP-11746-08.patch
          against trunk revision 3fb5abf.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 4 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 generated 1 release audit warnings.

          +1 core tests. The patch passed unit tests in .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6068//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/6068//artifact/patchprocess/patchReleaseAuditProblems.txt
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6068//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/12723454/HADOOP-11746-08.patch against trunk revision 3fb5abf. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 4 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 generated 1 release audit warnings. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6068//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/6068//artifact/patchprocess/patchReleaseAuditProblems.txt Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6068//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          -09:

          • -08 was the wrong file. woops.
          Show
          aw Allen Wittenauer added a comment - -09: -08 was the wrong file. woops.
          Hide
          aw Allen Wittenauer added a comment -

          -08:

          • borrow some code from the folks at HBase to help make the checkstyle check more useful
          • add an EOL whitespace check bz i'm tired of checking for it myself.
          • fix some extraneous output when no tests have failed
          Show
          aw Allen Wittenauer added a comment - -08: borrow some code from the folks at HBase to help make the checkstyle check more useful add an EOL whitespace check bz i'm tired of checking for it myself. fix some extraneous output when no tests have failed
          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/12709479/HADOOP-11746-07.patch
          against trunk revision 96d7211.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 3 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6065//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6065//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/12709479/HADOOP-11746-07.patch against trunk revision 96d7211. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 3 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6065//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6065//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          One thing I'm still confused by is why the original test-patch exit'd with 0 during some failure situations. Is this a bug in the original or was there a reason? I kept that behavior but it still seems wrong...

          Show
          aw Allen Wittenauer added a comment - One thing I'm still confused by is why the original test-patch exit'd with 0 during some failure situations. Is this a bug in the original or was there a reason? I kept that behavior but it still seems wrong...
          Hide
          aw Allen Wittenauer added a comment -

          -07:

          • fixes the duplicate test bit that was missing in -06
          Show
          aw Allen Wittenauer added a comment - -07: fixes the duplicate test bit that was missing in -06
          Hide
          aw Allen Wittenauer added a comment -

          Argh, just realized -06 doesn't have the uniq'ing of CHANGED_MODULES, so you'll see duplicate tests.

          Show
          aw Allen Wittenauer added a comment - Argh, just realized -06 doesn't have the uniq'ing of CHANGED_MODULES, so you'll see duplicate tests.
          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/12709449/HADOOP-11746-06.patch
          against trunk revision 4b3948e.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 3 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6064//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6064//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/12709449/HADOOP-11746-06.patch against trunk revision 4b3948e. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 3 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6064//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6064//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          -06:

          • lots of fixes and cleanup, including some minor perf fixes and removing a few needless temp files
          • On Solaris, use POSIX not SVID binaries
          • If a patch hits test-patch or smart-apply-path, short-circuit some pre-patch logic, then use the new versions as part of testing the patch
          Show
          aw Allen Wittenauer added a comment - -06: lots of fixes and cleanup, including some minor perf fixes and removing a few needless temp files On Solaris, use POSIX not SVID binaries If a patch hits test-patch or smart-apply-path, short-circuit some pre-patch logic, then use the new versions as part of testing the patch
          Hide
          cmccabe Colin P. McCabe added a comment - - edited

          Sean Busbey, have you tried out https://chrome.google.com/webstore/detail/git-patch-viewer/hkoggakcdopbgnaeeidcmopfekipkleg ? I find it really helpful for viewing diffs on JIRA.

          [edit: looking at this again, this may not be too helpful since this is mostly a rewrite, so having both the left and right sides is not that important.]

          Show
          cmccabe Colin P. McCabe added a comment - - edited Sean Busbey , have you tried out https://chrome.google.com/webstore/detail/git-patch-viewer/hkoggakcdopbgnaeeidcmopfekipkleg ? I find it really helpful for viewing diffs on JIRA. [edit: looking at this again, this may not be too helpful since this is mostly a rewrite, so having both the left and right sides is not that important.]
          Hide
          aw Allen Wittenauer added a comment -

          I actively try to avoid services that are so obviously compromised and not maintained since who knows what other problems may exist... iframe injections, etc. Plus, Hadoop uses JIRA as the platform for this. I understand that other projects use different tools, but we don't.

          Show
          aw Allen Wittenauer added a comment - I actively try to avoid services that are so obviously compromised and not maintained since who knows what other problems may exist... iframe injections, etc. Plus, Hadoop uses JIRA as the platform for this. I understand that other projects use different tools, but we don't.
          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/12708777/HADOOP-11746-05.patch
          against trunk revision ed72daa.

          -1 @author. The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 3 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6043//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6043//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/12708777/HADOOP-11746-05.patch against trunk revision ed72daa. -1 @author . The patch appears to contain 13 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 3 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6043//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6043//console This message is automatically generated.
          Hide
          busbey Sean Busbey added a comment -

          I don't follow. Presuming it is insecure, what could we lose by having the patch there? Is there another review tool that's more accessible than locally applying an putting comments here in Jira?

          Show
          busbey Sean Busbey added a comment - I don't follow. Presuming it is insecure, what could we lose by having the patch there? Is there another review tool that's more accessible than locally applying an putting comments here in Jira?
          Hide
          aw Allen Wittenauer added a comment -

          I pretty much don't trust ASF's reviewboard, given it has millions of spam accounts on it. I don't think it's secure at all.

          Show
          aw Allen Wittenauer added a comment - I pretty much don't trust ASF's reviewboard, given it has millions of spam accounts on it. I don't think it's secure at all.
          Hide
          busbey Sean Busbey added a comment -

          would you mind throwing this on reviewboard?

          Show
          busbey Sean Busbey added a comment - would you mind throwing this on reviewboard?
          Hide
          aw Allen Wittenauer added a comment -

          -05:

          • more major code cleanup: renamed quite a few functions, removed more cruft, etc
          • shelldoc capability docs
          • added a different table for test failures
          • slightly changed the overall +/-1 output
          • fixed some issues with relative paths
          • reworked some branch detection stuff
          • reworked the failed test output list to remove the org.apache part

          Need someone to review this now....

          Show
          aw Allen Wittenauer added a comment - -05: more major code cleanup: renamed quite a few functions, removed more cruft, etc shelldoc capability docs added a different table for test failures slightly changed the overall +/-1 output fixed some issues with relative paths reworked some branch detection stuff reworked the failed test output list to remove the org.apache part Need someone to review this now....
          Hide
          aw Allen Wittenauer added a comment - - edited

          Oh, also, smart-apply.sh has the same /tmp race issues (using the exact same file name as test-patch.sh). Hooray!

          Show
          aw Allen Wittenauer added a comment - - edited Oh, also, smart-apply.sh has the same /tmp race issues (using the exact same file name as test-patch.sh). Hooray!
          Hide
          aw Allen Wittenauer added a comment -

          Found two bugs this morning with the current patch, both of them minor:

          • http mode doesn't work because I forgot how regexp match worked in bash on that line.
          • in jenkins mode, we shouldn't try to cp the support dir lib if it doesn't exist.

          BTW, there's a good chance I've already fixed the race that Karthik Kambatla is hitting in YARN-3412. findmodules (which is the one that determines which pom.xml files to use for tests) was one of the first parts of the old code that I saw that scared me a bit. It features a hard-coded path that isn't tied to the current patch run directory at all and is global to the box. While it does use the pid for the name, given the right conditions (like, oh, something that spawns lots of child processes...), one could see a race there.

          FWIW, the current code races pretty hard at the directory level in developer mode. In Jenkins mode, this should be less of a concern unless Jenkins isn't able to clean up it's workspace directory.

          Show
          aw Allen Wittenauer added a comment - Found two bugs this morning with the current patch, both of them minor: http mode doesn't work because I forgot how regexp match worked in bash on that line. in jenkins mode, we shouldn't try to cp the support dir lib if it doesn't exist. BTW, there's a good chance I've already fixed the race that Karthik Kambatla is hitting in YARN-3412 . findmodules (which is the one that determines which pom.xml files to use for tests) was one of the first parts of the old code that I saw that scared me a bit. It features a hard-coded path that isn't tied to the current patch run directory at all and is global to the box. While it does use the pid for the name, given the right conditions (like, oh, something that spawns lots of child processes...), one could see a race there. FWIW, the current code races pretty hard at the directory level in developer mode. In Jenkins mode, this should be less of a concern unless Jenkins isn't able to clean up it's workspace directory.
          Hide
          aw Allen Wittenauer added a comment -

          -1 @author. The patch appears to contain 12 @author tags which the Hadoop community has agreed to not allow in code contributions.

          lol

          Show
          aw Allen Wittenauer added a comment - -1 @author. The patch appears to contain 12 @author tags which the Hadoop community has agreed to not allow in code contributions. lol
          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/12708341/HADOOP-11746-04.patch
          against trunk revision 85dc3c1.

          -1 @author. The patch appears to contain 12 @author tags which the Hadoop community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 3 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 .

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6030//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6030//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/12708341/HADOOP-11746-04.patch against trunk revision 85dc3c1. -1 @author . The patch appears to contain 12 @author tags which the Hadoop community has agreed to not allow in code contributions. +1 tests included . The patch appears to include 3 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 . Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/6030//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/6030//console This message is automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          I'm marking this as incompatible because the output changed.

          Show
          aw Allen Wittenauer added a comment - I'm marking this as incompatible because the output changed.
          Hide
          aw Allen Wittenauer added a comment - - edited

          -04:

          • MUCH better branch and issue detection on patches
          • checkstyle sort of works now!
          • add missing license files
          • some general code cleanup
          • JIRA comments are now formatted correctly
          • console comments had some display issues fixed
          • Removed a lot of "if jenkins this, if not jenkins this", simplifying the code paths
          • in addition to files, test-patch.sh now supports issues and http(s) URLs which will automatically download patches off of JIRA
          • give some hints about execution times while running too
          • fixed some issues around running tests

          To-do:

          • Add some ssssaaaaafffffffeeeeetttttttyyyyy checks.
          • Test on Jenkins
          • Do more broken patch tests
          • cleanup the function names, comment the code more, etc.
          • Any other requests?
          Show
          aw Allen Wittenauer added a comment - - edited -04: MUCH better branch and issue detection on patches checkstyle sort of works now! add missing license files some general code cleanup JIRA comments are now formatted correctly console comments had some display issues fixed Removed a lot of "if jenkins this, if not jenkins this", simplifying the code paths in addition to files, test-patch.sh now supports issues and http(s) URLs which will automatically download patches off of JIRA give some hints about execution times while running too fixed some issues around running tests To-do: Add some ssssaaaaafffffffeeeeetttttttyyyyy checks. Test on Jenkins Do more broken patch tests cleanup the function names, comment the code more, etc. Any other requests?
          Hide
          aw Allen Wittenauer added a comment -

          Setting HADOOP-11778 as required for checkstyle support, based upon my current experiments.

          Show
          aw Allen Wittenauer added a comment - Setting HADOOP-11778 as required for checkstyle support, based upon my current experiments.
          Hide
          aw Allen Wittenauer added a comment - - edited

          Current shell-output. (Note that HADOOP-11746 is the name of the git branch where I ran the test-patch script)

          -1 overall
          
          | Vote |           Subsystem |  Runtime  | Comment
          |   0  |          pre-patch  |  09m 30s  |  Pre-patch HADOOP-11746 compilation 
          |      |                     |           | is healthy.
          |  +1  |            @author  |  00m 00s  |  The patch does not contain any 
          |      |                     |           | @author tags.
          |  -1  |     tests included  |  00m 00s  |  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  |  04m 22s  |  There were no new javac warning 
          |      |                     |           | messages.
          |  +1  |            javadoc  |  06m 16s  |  There were no new javadoc warning 
          |      |                     |           | messages.
          |  -1  |      release audit  |  00m 09s  |  The applied patch generated 3 
          |      |                     |           | release audit warnings.
          |  +1  |         checkstyle  |  03m 04s  |  There were no new checkstyle issues. 
          |  +1  |         shellcheck  |  00m 02s  |  There were no new shellcheck issues. 
          |  +1  |            install  |  02m 31s  |  mvn install still works. 
          |  +1  |    eclipse:eclipse  |  00m 24s  |  The patch built with 
          |      |                     |           | eclipse:eclipse.
          
          
          || Subsystem || Report ||
          | Release Audit | /tmp/hadoop-test-patch/14934/patchReleaseAuditProblems.txt |
          

          The current jira format has some minor issues. Fixing the two missing new lines and the missing backslash results in:

          -1 overall

          Vote Subsystem Runtime Comment
          0 pre-patch 09m 30s Pre-patch HADOOP-11746 compilation is healthy.
          +1 @author 00m 00s The patch does not contain any @author tags.
          -1 tests included 00m 00s 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 04m 22s There were no new javac warning messages.
          +1 javadoc 06m 16s There were no new javadoc warning messages.
          -1 release audit 00m 09s The applied patch generated 3 release audit warnings.
          +1 checkstyle 03m 04s There were no new checkstyle issues.
          +1 shellcheck 00m 02s There were no new shellcheck issues.
          +1 install 02m 31s mvn install still works.
          +1 eclipse:eclipse 00m 24s The patch built with eclipse:eclipse.


          Subsystem Report
          Release Audit /artifact/patchprocess/patchReleaseAuditProblems.txt
          Console output /artifact/patchprocess/console

          This message was automatically generated.

          Show
          aw Allen Wittenauer added a comment - - edited Current shell-output. (Note that HADOOP-11746 is the name of the git branch where I ran the test-patch script) -1 overall | Vote | Subsystem | Runtime | Comment | 0 | pre-patch | 09m 30s | Pre-patch HADOOP-11746 compilation | | | | is healthy. | +1 | @author | 00m 00s | The patch does not contain any | | | | @author tags. | -1 | tests included | 00m 00s | 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 | 04m 22s | There were no new javac warning | | | | messages. | +1 | javadoc | 06m 16s | There were no new javadoc warning | | | | messages. | -1 | release audit | 00m 09s | The applied patch generated 3 | | | | release audit warnings. | +1 | checkstyle | 03m 04s | There were no new checkstyle issues. | +1 | shellcheck | 00m 02s | There were no new shellcheck issues. | +1 | install | 02m 31s | mvn install still works. | +1 | eclipse:eclipse | 00m 24s | The patch built with | | | | eclipse:eclipse. || Subsystem || Report || | Release Audit | /tmp/hadoop-test-patch/14934/patchReleaseAuditProblems.txt | The current jira format has some minor issues. Fixing the two missing new lines and the missing backslash results in: -1 overall Vote Subsystem Runtime Comment 0 pre-patch 09m 30s Pre-patch HADOOP-11746 compilation is healthy. +1 @author 00m 00s The patch does not contain any @author tags. -1 tests included 00m 00s 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 04m 22s There were no new javac warning messages. +1 javadoc 06m 16s There were no new javadoc warning messages. -1 release audit 00m 09s The applied patch generated 3 release audit warnings. +1 checkstyle 03m 04s There were no new checkstyle issues. +1 shellcheck 00m 02s There were no new shellcheck issues. +1 install 02m 31s mvn install still works. +1 eclipse:eclipse 00m 24s The patch built with eclipse:eclipse. Subsystem Report Release Audit /artifact/patchprocess/patchReleaseAuditProblems.txt Console output /artifact/patchprocess/console This message was automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          -03:

          • Beginnings of plug-in support, including some very alpha checkstyle and shellcheck plug-ins
          • --debug support
          • Branch detection for --dirty-workspace and the ability to work on branches that aren't trunk (note: must conform to HowToContribute wiki!)
          • Some more output cleanup/enhancements
          • re-arrange/label the usage/help message output
          • re-order the basic tests a bit
          • include some status/timings for things that are normally hidden
          • imported some routines from hadoop-funcions.sh, so the start of some shelldoc.py support

          Any hints on how I can start testing under jenkins? How do I duplicate what we currently have?

          Show
          aw Allen Wittenauer added a comment - -03: Beginnings of plug-in support, including some very alpha checkstyle and shellcheck plug-ins --debug support Branch detection for --dirty-workspace and the ability to work on branches that aren't trunk (note: must conform to HowToContribute wiki!) Some more output cleanup/enhancements re-arrange/label the usage/help message output re-order the basic tests a bit include some status/timings for things that are normally hidden imported some routines from hadoop-funcions.sh, so the start of some shelldoc.py support Any hints on how I can start testing under jenkins? How do I duplicate what we currently have?
          Hide
          aw Allen Wittenauer added a comment - - edited

          -02:

          • still very little new functionality, except that --dirty-workspace is now working for me.
          • console output cleanup
          • fix the /tmp default for the patch scratch space to be /tmp/(projname)-test-patch/pid to allow for simultaneous test patch runs
          • add in a timer to show how long various steps take (as requested by Ravi Prakash )
          • Fix more "we should have a var rather than hard code this binary" issues
          • just skip the findbugs test if findbugs isn't installed
          • send the mvn install run to a log file rather than dumping it to the screen
          • fix some of the backslash indentation problems generated by the autoformatter

          Current console output now looks like:

          w$ dev-support/test-patch.sh --dirty-workspace /tmp/H1
          Running in developer mode
          /tmp/Hadoop-test-patch/54448 has been created
          
          
          =======================================================================
          =======================================================================
                                  Testing patch for H1.
          =======================================================================
          =======================================================================
          ....
          -1 overall
          
          | Vote |           Subsystem | Comment
          |  +1  |            @author  |  00m 00s  | The patch does not contain any 
                                                   | @author tags.
          |  -1  |     tests included  |  00m 00s  | 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  |  04m 30s  | There were no new javac warning 
                                                   | messages.
          |  +1  |            javadoc  |  06m 15s  | There were no new javadoc warning 
                                                   | messages.
          |  +1  |    eclipse:eclipse  |  00m 24s  | The patch built with eclipse:eclipse.
          |  -1  |      release audit  |  00m 05s  | The applied patch generated 1 release 
                                                   | audit warnings.
          
          
          =======================================================================
          =======================================================================
                                     Finished build.
          =======================================================================
          =======================================================================
          

          Note the always centered text and the column wrap on the output.

          Show
          aw Allen Wittenauer added a comment - - edited -02: still very little new functionality, except that --dirty-workspace is now working for me. console output cleanup fix the /tmp default for the patch scratch space to be /tmp/(projname)-test-patch/pid to allow for simultaneous test patch runs add in a timer to show how long various steps take (as requested by Ravi Prakash ) Fix more "we should have a var rather than hard code this binary" issues just skip the findbugs test if findbugs isn't installed send the mvn install run to a log file rather than dumping it to the screen fix some of the backslash indentation problems generated by the autoformatter Current console output now looks like: w$ dev-support/test-patch.sh --dirty-workspace /tmp/H1 Running in developer mode /tmp/Hadoop-test-patch/54448 has been created ======================================================================= ======================================================================= Testing patch for H1. ======================================================================= ======================================================================= .... -1 overall | Vote | Subsystem | Comment | +1 | @author | 00m 00s | The patch does not contain any | @author tags. | -1 | tests included | 00m 00s | 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 | 04m 30s | There were no new javac warning | messages. | +1 | javadoc | 06m 15s | There were no new javadoc warning | messages. | +1 | eclipse:eclipse | 00m 24s | The patch built with eclipse:eclipse. | -1 | release audit | 00m 05s | The applied patch generated 1 release | audit warnings. ======================================================================= ======================================================================= Finished build. ======================================================================= ======================================================================= Note the always centered text and the column wrap on the output.
          Hide
          aw Allen Wittenauer added a comment -

          I see a few off by one errors. Woops.

          Show
          aw Allen Wittenauer added a comment - I see a few off by one errors. Woops.
          Hide
          aw Allen Wittenauer added a comment -

          -01:

          • start preparing for set -euo pipefail mode (hopeful thinking)
          • complain if JAVA_HOME isn't found since some of the maven tests fail if it isn't defined
          • JIRA output now uses more markup, because tables are cool
          • rework the generic console output header bits to auto-center
          • rework the error report based upon the new JIRA output, including splitting it out into its own outputter
          • add a few helper routines to auto center text, etc.
          • only find the changed modules once instead of twice
          • run it through a shell code formatter, but I really hate how it handles backslashed lines
          • lots of "oh we have a variable for this binary, but let's ignore it and call whatever is in the path anyway" fixes
          • fix a few inconsistencies in how we checked and reported errors
          • updated quite a few things to modern bash semantics
          • local and lowercase'd all of the local vars
          • removed forrest bits

          I haven't done a full run yet, never mind against Jenkins, but this one might actually work. Still a lot to do, including making it pluggable and moving some of these bits into that plug-in dir.

          Show
          aw Allen Wittenauer added a comment - -01: start preparing for set -euo pipefail mode (hopeful thinking) complain if JAVA_HOME isn't found since some of the maven tests fail if it isn't defined JIRA output now uses more markup, because tables are cool rework the generic console output header bits to auto-center rework the error report based upon the new JIRA output, including splitting it out into its own outputter add a few helper routines to auto center text, etc. only find the changed modules once instead of twice run it through a shell code formatter, but I really hate how it handles backslashed lines lots of "oh we have a variable for this binary, but let's ignore it and call whatever is in the path anyway" fixes fix a few inconsistencies in how we checked and reported errors updated quite a few things to modern bash semantics local and lowercase'd all of the local vars removed forrest bits I haven't done a full run yet, never mind against Jenkins, but this one might actually work. Still a lot to do, including making it pluggable and moving some of these bits into that plug-in dir.
          Hide
          aw Allen Wittenauer added a comment - - edited

          Nah. Probably not worth it.

          FWIW, the biggest problems are mixing up string and arithmetic comparison operators.

          Show
          aw Allen Wittenauer added a comment - - edited Nah. Probably not worth it. FWIW, the biggest problems are mixing up string and arithmetic comparison operators.
          Hide
          busbey Sean Busbey added a comment -

          Just heads up, I'd like to coalesce the test-patch process for both Hadoop and HBase. My planned start wasn't until April.

          I'll try applying similar changes to the HBase fork as you develop here. Are the subtle bugs enough that I should do a pass of that now?

          Show
          busbey Sean Busbey added a comment - Just heads up, I'd like to coalesce the test-patch process for both Hadoop and HBase. My planned start wasn't until April. I'll try applying similar changes to the HBase fork as you develop here. Are the subtle bugs enough that I should do a pass of that now?
          Hide
          aw Allen Wittenauer added a comment -

          -00:

          • initial pass that just fixes the vast majority of shellcheck errors
          Show
          aw Allen Wittenauer added a comment - -00: initial pass that just fixes the vast majority of shellcheck errors
          Hide
          aw Allen Wittenauer added a comment -

          I'm going to go for bash for now, simply because there would be a ton of shell outs if we did it in python to the point that half the code would be bash anyway. I might write some helper code in python to replace the wget though. That would be significantly better to fetch from JIRA via REST.

          As it is, just running shellcheck on the existing code has found quite a few subtle bugs.

          Show
          aw Allen Wittenauer added a comment - I'm going to go for bash for now, simply because there would be a ton of shell outs if we did it in python to the point that half the code would be bash anyway. I might write some helper code in python to replace the wget though. That would be significantly better to fetch from JIRA via REST. As it is, just running shellcheck on the existing code has found quite a few subtle bugs.
          Hide
          gkesavan Giridharan Kesavan added a comment -

          Allen Wittenauer are you planning to re-write the test-patch.sh in python?
          I'm interested in helping in any means, writing the python code or testing it out. please let me know.

          Show
          gkesavan Giridharan Kesavan added a comment - Allen Wittenauer are you planning to re-write the test-patch.sh in python? I'm interested in helping in any means, writing the python code or testing it out. please let me know.

            People

            • Assignee:
              aw Allen Wittenauer
              Reporter:
              aw Allen Wittenauer
            • Votes:
              2 Vote for this issue
              Watchers:
              20 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development