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

add an entry point that can start any Yarn service

    Details

    • Type: New Feature
    • Status: Patch Available
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 2.8.0
    • Fix Version/s: None
    • Component/s: api
    • Labels:
    • Target Version/s:

      Description

      There's no need to write separate .main classes for every Yarn service, given that the startup mechanism should be identical: create, init, start, wait for stopped -with an interrupt handler to trigger a clean shutdown on a control-c interrupt.

      Provide one that takes any classname, and a list of config files/options

      1. org.apache.hadoop.servic...mon 3.0.0-SNAPSHOT API).pdf
        401 kB
        Steve Loughran
      2. YARN-679-001.patch
        13 kB
        Steve Loughran
      3. YARN-679-002.patch
        176 kB
        Steve Loughran
      4. YARN-679-002.patch
        176 kB
        Steve Loughran
      5. YARN-679-003.patch
        186 kB
        Steve Loughran
      6. YARN-679-004.patch
        196 kB
        Steve Loughran
      7. YARN-679-005.patch
        189 kB
        Steve Loughran
      8. YARN-679-006.patch
        190 kB
        Steve Loughran
      9. YARN-679-007.patch
        190 kB
        Steve Loughran
      10. YARN-679-008.patch
        190 kB
        Steve Loughran
      11. YARN-679-009.patch
        193 kB
        Steve Loughran
      12. YARN-679-010.patch
        194 kB
        Steve Loughran
      13. YARN-679-011.patch
        193 kB
        Steve Loughran

        Issue Links

          Activity

          Hide
          stevel@apache.org Steve Loughran added a comment -

          This is to show Vinod why Service.waitForServiceToStop() is useful -an interrupt handler hooked in to ^C calls give the service 30s grace before exiting

          Show
          stevel@apache.org Steve Loughran added a comment - This is to show Vinod why Service.waitForServiceToStop() is useful -an interrupt handler hooked in to ^C calls give the service 30s grace before exiting
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I've been evolving this driven by the Hoya application; code is up online at
          https://github.com/hortonworks/hoya/tree/master/src/main/java/org/apache/hadoop/yarn/service/launcher

          Some observations
          I added an interface GetExceptionExitCode to get an exception code off any Exception.

          public interface GetExceptionExitCode {
          
            int  getExitCode();
          }
          

          It'd be nice to have this interface implemented by Shell.ExitCodeException and ExitUtil.ExitException so that we have a consistent way to get exit codes from any exception willing to provide them.

          ps. we could do with a more standardised set of error codes for YARN applications -convention rather than mandatory.

          To make services executable, rather than just deployable, I added another interface.

          public interface RunService {
          
            /**
             * Propagate the command line arguments
             * 
             * @param args argument list
             * @throws IOException any problem
             */
            void setArgs(String...args) throws Exception;
            
            /**
             * Run a service
             * @return the exit code
             * @throws Throwable any exception to report
             */
            int runService() throws Throwable ;
          }
          

          Here setArgs passes down all the arguments *before Service.init(Config) is called. This lets me tune the config passed to the superclass based on the supplied arguments.

          runService() is called after Service.start().

          The model here is that the main() thread goes

          1. create service class
          2. setArgs(...)
          3. init(config
          4. start()
          5. int exit=runService()
          6. stop

          The service doesn't need to start its own worker thread, and the exit code from runService becomes the exit code of the app. Any of the service methods are also free to throw an exception; it implements getExitCode() that becomes the exit code of the app.

          The code seems a bit over-complex, but it's evolved to also be the entry point for tests too

          Show
          stevel@apache.org Steve Loughran added a comment - I've been evolving this driven by the Hoya application; code is up online at https://github.com/hortonworks/hoya/tree/master/src/main/java/org/apache/hadoop/yarn/service/launcher Some observations I added an interface GetExceptionExitCode to get an exception code off any Exception. public interface GetExceptionExitCode { int getExitCode(); } It'd be nice to have this interface implemented by Shell.ExitCodeException and ExitUtil.ExitException so that we have a consistent way to get exit codes from any exception willing to provide them. ps. we could do with a more standardised set of error codes for YARN applications -convention rather than mandatory. To make services executable, rather than just deployable, I added another interface. public interface RunService { /** * Propagate the command line arguments * * @param args argument list * @ throws IOException any problem */ void setArgs( String ...args) throws Exception; /** * Run a service * @ return the exit code * @ throws Throwable any exception to report */ int runService() throws Throwable ; } Here setArgs passes down all the arguments *before Service.init(Config) is called. This lets me tune the config passed to the superclass based on the supplied arguments. runService() is called after Service.start() . The model here is that the main() thread goes create service class setArgs(...) init(config start() int exit=runService() stop The service doesn't need to start its own worker thread, and the exit code from runService becomes the exit code of the app. Any of the service methods are also free to throw an exception; it implements getExitCode() that becomes the exit code of the app. The code seems a bit over-complex, but it's evolved to also be the entry point for tests too
          Hide
          acmurthy Arun C Murthy added a comment -

          +1 for the direction! Thanks Steve Loughran!

          Show
          acmurthy Arun C Murthy added a comment - +1 for the direction! Thanks Steve Loughran !
          Hide
          stevel@apache.org Steve Loughran added a comment -

          This turns out to be useful both client-side and server-side. as any client that directly subclasses {{YarnClientImpl }} or hosts it within its own service composite can become a launched service. Similarly, AMs and containers are/contain YARN services, and need their own entry points.

          Having a single entry point means more effort can be put in to having an entry point, rather than per-service ones that are implemented by cut-and-paste and may be under-maintained.

          1. effectively guarantees a well-tested shutdown/interrupt handler
          2. effectively guarantees more functional testing of failure paths
          3. with a good factoring of operations it also enables good unit test coverage
          4. makes it easier to write new YARN services
          5. provides a standard base set of exit codes
          6. allows for a single entry point script to directly create and run YARN services
          Show
          stevel@apache.org Steve Loughran added a comment - This turns out to be useful both client-side and server-side. as any client that directly subclasses {{YarnClientImpl }} or hosts it within its own service composite can become a launched service. Similarly, AMs and containers are/contain YARN services, and need their own entry points. Having a single entry point means more effort can be put in to having an entry point, rather than per-service ones that are implemented by cut-and-paste and may be under-maintained. effectively guarantees a well-tested shutdown/interrupt handler effectively guarantees more functional testing of failure paths with a good factoring of operations it also enables good unit test coverage makes it easier to write new YARN services provides a standard base set of exit codes allows for a single entry point script to directly create and run YARN services
          Hide
          stevel@apache.org Steve Loughran added a comment -

          This (big) patch provides the service launcher with

          1. a ServiceLauncher class to load and execute any service
          2. a new LaunchableService extends Service interface that adds two methods: bindArgs() and execute(), to bind the CLI args and to execute the service. This improves service integration with the launcher, but is not mandatory. Any service with an empty or string constructor can be launched.
          3. Signal hooking for best-effort shutdown
          4. an exception handling model which allows any exception to generate a return code.
          5. a standard set of return codes LauncherExitCodes
          6. changes to ExitUtil to improve its ability to exit with given exit codes and messages.
          7. lots and lots of tests
          8. lots and lots of javadocs.

          The javadocs provide the most information; after a mvn javadoc:javadoc run, look in hadoop-common-project/hadoop-common/target/site/api/org/apache/hadoop/service/launcher/package-summary.html

          This is the service launcher we've been using for 8+ months, putting into hadoop common has consisted mainly of tests & javadocs, with some (long desired) enhancements to exit code handling (HADOOP-9626).

          I have not yet made any changes to CLI processing. So far it doesn't do as much as tool runner. All it does is scan the CLI args list for --conf filename pairs, extracting configuration files from them and, after testing, adds it to the configuration used to initialize the service.

          If we want more, rather than try and get the (outdated) GenericOptionsParser to do the parsing, I'd prefer to write a JCommander-based alternative, with (most) of the same arguments, but more usable and extensible, and with its own tests. I've been using JCommander and an familiar with its features and quirks now.

          Show
          stevel@apache.org Steve Loughran added a comment - This (big) patch provides the service launcher with a ServiceLauncher class to load and execute any service a new LaunchableService extends Service interface that adds two methods: bindArgs() and execute() , to bind the CLI args and to execute the service. This improves service integration with the launcher, but is not mandatory . Any service with an empty or string constructor can be launched. Signal hooking for best-effort shutdown an exception handling model which allows any exception to generate a return code. a standard set of return codes LauncherExitCodes changes to ExitUtil to improve its ability to exit with given exit codes and messages. lots and lots of tests lots and lots of javadocs. The javadocs provide the most information; after a mvn javadoc:javadoc run, look in hadoop-common-project/hadoop-common/target/site/api/org/apache/hadoop/service/launcher/package-summary.html This is the service launcher we've been using for 8+ months, putting into hadoop common has consisted mainly of tests & javadocs, with some (long desired) enhancements to exit code handling ( HADOOP-9626 ). I have not yet made any changes to CLI processing. So far it doesn't do as much as tool runner. All it does is scan the CLI args list for --conf filename pairs, extracting configuration files from them and, after testing, adds it to the configuration used to initialize the service. If we want more, rather than try and get the (outdated) GenericOptionsParser to do the parsing, I'd prefer to write a JCommander-based alternative, with (most) of the same arguments, but more usable and extensible, and with its own tests. I've been using JCommander and an familiar with its features and quirks now.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          This is the package javadoc

          Show
          stevel@apache.org Steve Loughran added a comment - This is the package javadoc
          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/12648651/org.apache.hadoop.servic...mon%203.0.0-SNAPSHOT%20API%29.pdf
          against trunk revision .

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/3919//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/12648651/org.apache.hadoop.servic...mon%203.0.0-SNAPSHOT%20API%29.pdf against trunk revision . -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-YARN-Build/3919//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/12648697/YARN-679-002.patch
          against trunk revision .

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

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

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

          -1 javadoc. The javadoc tool appears to have generated 5 warning messages.
          See https://builds.apache.org/job/PreCommit-YARN-Build/3926//artifact/trunk/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 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

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

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/3926//testReport/
          Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/3926//artifact/trunk/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/3926//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/12648697/YARN-679-002.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 31 new or modified test files. -1 javac . The applied patch generated 1285 javac compiler warnings (more than the trunk's current 1276 warnings). -1 javadoc . The javadoc tool appears to have generated 5 warning messages. See https://builds.apache.org/job/PreCommit-YARN-Build/3926//artifact/trunk/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 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/3926//testReport/ Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/3926//artifact/trunk/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-YARN-Build/3926//console This message is automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          some of the compiler warning are about use of sun.misc for interrupt handling -that's all in one class to avoid spreading complaints. We could actually move signal handlers to using that wrapper for registry/callback, and reduce the number of warnings...

          Show
          stevel@apache.org Steve Loughran added a comment - some of the compiler warning are about use of sun.misc for interrupt handling -that's all in one class to avoid spreading complaints. We could actually move signal handlers to using that wrapper for registry/callback, and reduce the number of warnings...
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Revised patch

          1. uses reflection to load the HDFS and YARN configurations if present -so forcing in their resources
          2. uses GenericOptionsParser to parse the options -so the command line is now consistent with ToolRunner. (There's one extra constraint -that all configs resolve to valid paths in the filesystem)
          3. GenericOptionsParser adds a flag to indicate whether or not the parse worked...until now it looks like an invalid set of generic options could still get handed down to the tool
          Show
          stevel@apache.org Steve Loughran added a comment - Revised patch uses reflection to load the HDFS and YARN configurations if present -so forcing in their resources uses GenericOptionsParser to parse the options -so the command line is now consistent with ToolRunner. (There's one extra constraint -that all configs resolve to valid paths in the filesystem) GenericOptionsParser adds a flag to indicate whether or not the parse worked...until now it looks like an invalid set of generic options could still get handed down to the tool
          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/12653051/YARN-679-003.patch
          against trunk revision .

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

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

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

          -1 javadoc. The javadoc tool appears to have generated 5 warning messages.
          See https://builds.apache.org/job/PreCommit-YARN-Build/4133//artifact/trunk/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 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

          org.apache.hadoop.service.launcher.TestServiceLaunchedRunning
          org.apache.hadoop.service.launcher.TestServiceLaunchNoArgsAllowed

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

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/4133//testReport/
          Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/4133//artifact/trunk/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/4133//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/12653051/YARN-679-003.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 31 new or modified test files. -1 javac . The applied patch generated 1267 javac compiler warnings (more than the trunk's current 1258 warnings). -1 javadoc . The javadoc tool appears to have generated 5 warning messages. See https://builds.apache.org/job/PreCommit-YARN-Build/4133//artifact/trunk/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 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.service.launcher.TestServiceLaunchedRunning org.apache.hadoop.service.launcher.TestServiceLaunchNoArgsAllowed +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/4133//testReport/ Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/4133//artifact/trunk/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-YARN-Build/4133//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 14m 44s Pre-patch trunk compilation is healthy.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 tests included 0m 0s The patch appears to include 31 new or modified test files.
          -1 javac 7m 32s The applied patch generated 130 additional warning messages.
          -1 javadoc 9m 33s The applied patch generated 3 additional warning messages.
          +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings.
          -1 checkstyle 1m 6s The applied patch generated 150 new checkstyle issues (total was 140, now 287).
          -1 whitespace 0m 7s The patch has 5 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 34s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          +1 findbugs 1m 40s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          -1 common tests 22m 21s Tests failed in hadoop-common.
              59m 38s  



          Reason Tests
          Failed unit tests hadoop.service.launcher.TestServiceLaunchNoArgsAllowed
            hadoop.service.launcher.TestServiceLaunchedRunning



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12653051/YARN-679-003.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 6ae2a0d
          javac https://builds.apache.org/job/PreCommit-YARN-Build/7664/artifact/patchprocess/diffJavacWarnings.txt
          javadoc https://builds.apache.org/job/PreCommit-YARN-Build/7664/artifact/patchprocess/diffJavadocWarnings.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7664/artifact/patchprocess/diffcheckstylehadoop-common.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/7664/artifact/patchprocess/whitespace.txt
          hadoop-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7664/artifact/patchprocess/testrun_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7664/testReport/
          Java 1.7.0_55
          uname Linux asf905.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/7664/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 44s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 31 new or modified test files. -1 javac 7m 32s The applied patch generated 130 additional warning messages. -1 javadoc 9m 33s The applied patch generated 3 additional warning messages. +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 1m 6s The applied patch generated 150 new checkstyle issues (total was 140, now 287). -1 whitespace 0m 7s The patch has 5 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 34s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 1m 40s The patch does not introduce any new Findbugs (version 2.0.3) warnings. -1 common tests 22m 21s Tests failed in hadoop-common.     59m 38s   Reason Tests Failed unit tests hadoop.service.launcher.TestServiceLaunchNoArgsAllowed   hadoop.service.launcher.TestServiceLaunchedRunning Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12653051/YARN-679-003.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 6ae2a0d javac https://builds.apache.org/job/PreCommit-YARN-Build/7664/artifact/patchprocess/diffJavacWarnings.txt javadoc https://builds.apache.org/job/PreCommit-YARN-Build/7664/artifact/patchprocess/diffJavadocWarnings.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7664/artifact/patchprocess/diffcheckstylehadoop-common.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/7664/artifact/patchprocess/whitespace.txt hadoop-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7664/artifact/patchprocess/testrun_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7664/testReport/ Java 1.7.0_55 uname Linux asf905.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/7664/console This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 14m 41s Pre-patch trunk compilation is healthy.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 tests included 0m 0s The patch appears to include 31 new or modified test files.
          -1 javac 7m 34s The applied patch generated 128 additional warning messages.
          +1 javadoc 9m 32s There were no new javadoc warning messages.
          +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings.
          -1 checkstyle 1m 3s The applied patch generated 150 new checkstyle issues (total was 140, now 287).
          -1 whitespace 0m 8s The patch has 5 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 32s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          +1 findbugs 1m 40s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          -1 common tests 23m 5s Tests failed in hadoop-common.
              60m 16s  



          Reason Tests
          Failed unit tests hadoop.service.launcher.TestServiceLaunchedRunning
            hadoop.service.launcher.TestServiceLaunchNoArgsAllowed



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12653051/YARN-679-003.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 0d6aa5d
          javac https://builds.apache.org/job/PreCommit-YARN-Build/7692/artifact/patchprocess/diffJavacWarnings.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7692/artifact/patchprocess/diffcheckstylehadoop-common.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/7692/artifact/patchprocess/whitespace.txt
          hadoop-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7692/artifact/patchprocess/testrun_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7692/testReport/
          Java 1.7.0_55
          uname Linux asf901.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/7692/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 41s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 31 new or modified test files. -1 javac 7m 34s The applied patch generated 128 additional warning messages. +1 javadoc 9m 32s There were no new javadoc warning messages. +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 1m 3s The applied patch generated 150 new checkstyle issues (total was 140, now 287). -1 whitespace 0m 8s The patch has 5 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 32s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 1m 40s The patch does not introduce any new Findbugs (version 2.0.3) warnings. -1 common tests 23m 5s Tests failed in hadoop-common.     60m 16s   Reason Tests Failed unit tests hadoop.service.launcher.TestServiceLaunchedRunning   hadoop.service.launcher.TestServiceLaunchNoArgsAllowed Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12653051/YARN-679-003.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 0d6aa5d javac https://builds.apache.org/job/PreCommit-YARN-Build/7692/artifact/patchprocess/diffJavacWarnings.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7692/artifact/patchprocess/diffcheckstylehadoop-common.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/7692/artifact/patchprocess/whitespace.txt hadoop-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7692/artifact/patchprocess/testrun_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7692/testReport/ Java 1.7.0_55 uname Linux asf901.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/7692/console This message was automatically generated.
          Hide
          gtCarrera9 Li Lu added a comment -

          Hi Steve Loughran, thanks for working on this. I have one question about applying the patch. I saw Jenkins successfully applied the latest patch two days ago, but when I tried to apply it today there were some errors. Since it's not very likely for a recent change to break this patch, could you please verify this patch with the latest trunk? Thanks!

          Show
          gtCarrera9 Li Lu added a comment - Hi Steve Loughran , thanks for working on this. I have one question about applying the patch. I saw Jenkins successfully applied the latest patch two days ago, but when I tried to apply it today there were some errors. Since it's not very likely for a recent change to break this patch, could you please verify this patch with the latest trunk? Thanks!
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I haven't gone near this recently; I'd like the workflow stuff to go in first as then they can hook up in order

          Show
          stevel@apache.org Steve Loughran added a comment - I haven't gone near this recently; I'd like the workflow stuff to go in first as then they can hook up in order
          Hide
          stevel@apache.org Steve Loughran added a comment -

          BTW, we've been using a precursor to this in Slider happily; this is what the entry point to the client looks like; the AM looks similar

          public class Slider extends SliderClient {
          
          
            public static final String SERVICE_CLASSNAME = "org.apache.slider.Slider";
          
            /**
             * This is the main entry point for the service launcher.
             * @param args command line arguments.
             */
            public static void main(String[] args) {
              
              //turn the args to a list
              List<String> argsList = Arrays.asList(args);
              //create a new list, as the ArrayList type doesn't push() on an insert
              List<String> extendedArgs = new ArrayList<String>(argsList);
              //insert the service name
              extendedArgs.add(0, SERVICE_CLASSNAME);
              //now have the service launcher do its work
              ServiceLauncher.serviceMain(extendedArgs);
            }
          
          }
          

          This handles all the setup, parsing, exit reporting, shutdown hook registration, interrupt registration with escalation on a double signal, etc, etc. Stuff you need, especially in the AM.

          One thing the patch here does is uses org.apache.commons.cli for CLI work. Slider uses JCommander which is way, way better -it uses introspection to configure classes, e.g ActionPackageArgs. These structs become the same ones that the test runner and anything using it as a library use to execute operations, giving us a nicely extensible API (we just add new fields) and very simple parsing.

          I've not got jcommander stuff in this patch, because it will only add another dependency.

          Show
          stevel@apache.org Steve Loughran added a comment - BTW, we've been using a precursor to this in Slider happily; this is what the entry point to the client looks like; the AM looks similar public class Slider extends SliderClient { public static final String SERVICE_CLASSNAME = "org.apache.slider.Slider" ; /** * This is the main entry point for the service launcher. * @param args command line arguments. */ public static void main( String [] args) { //turn the args to a list List< String > argsList = Arrays.asList(args); //create a new list, as the ArrayList type doesn't push() on an insert List< String > extendedArgs = new ArrayList< String >(argsList); //insert the service name extendedArgs.add(0, SERVICE_CLASSNAME); //now have the service launcher do its work ServiceLauncher.serviceMain(extendedArgs); } } This handles all the setup, parsing, exit reporting, shutdown hook registration, interrupt registration with escalation on a double signal, etc, etc. Stuff you need, especially in the AM. One thing the patch here does is uses org.apache.commons.cli for CLI work. Slider uses JCommander which is way, way better -it uses introspection to configure classes, e.g ActionPackageArgs . These structs become the same ones that the test runner and anything using it as a library use to execute operations, giving us a nicely extensible API (we just add new fields) and very simple parsing. I've not got jcommander stuff in this patch, because it will only add another dependency.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 16m 56s Pre-patch trunk compilation is healthy.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 tests included 0m 0s The patch appears to include 31 new or modified test files.
          -1 javac 7m 51s The applied patch generated 8 additional warning messages.
          +1 javadoc 9m 41s There were no new javadoc warning messages.
          +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings.
          -1 checkstyle 1m 6s The applied patch generated 165 new checkstyle issues (total was 140, now 302).
          -1 whitespace 0m 9s The patch has 5 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 20s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          +1 findbugs 1m 51s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
          -1 common tests 22m 29s Tests failed in hadoop-common.
              62m 25s  



          Reason Tests
          Failed unit tests hadoop.net.TestNetUtils
            hadoop.ha.TestZKFailoverController
            hadoop.service.launcher.TestServiceLaunchedRunning
            hadoop.service.launcher.TestServiceLaunchNoArgsAllowed



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12750874/YARN-679-004.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / c77bd6a
          javac https://builds.apache.org/job/PreCommit-YARN-Build/8866/artifact/patchprocess/diffJavacWarnings.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/8866/artifact/patchprocess/diffcheckstylehadoop-common.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/8866/artifact/patchprocess/whitespace.txt
          hadoop-common test log https://builds.apache.org/job/PreCommit-YARN-Build/8866/artifact/patchprocess/testrun_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/8866/testReport/
          Java 1.7.0_55
          uname Linux asf901.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/8866/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 16m 56s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 31 new or modified test files. -1 javac 7m 51s The applied patch generated 8 additional warning messages. +1 javadoc 9m 41s There were no new javadoc warning messages. +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 1m 6s The applied patch generated 165 new checkstyle issues (total was 140, now 302). -1 whitespace 0m 9s The patch has 5 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 20s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 1m 51s The patch does not introduce any new Findbugs (version 3.0.0) warnings. -1 common tests 22m 29s Tests failed in hadoop-common.     62m 25s   Reason Tests Failed unit tests hadoop.net.TestNetUtils   hadoop.ha.TestZKFailoverController   hadoop.service.launcher.TestServiceLaunchedRunning   hadoop.service.launcher.TestServiceLaunchNoArgsAllowed Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12750874/YARN-679-004.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / c77bd6a javac https://builds.apache.org/job/PreCommit-YARN-Build/8866/artifact/patchprocess/diffJavacWarnings.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/8866/artifact/patchprocess/diffcheckstylehadoop-common.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/8866/artifact/patchprocess/whitespace.txt hadoop-common test log https://builds.apache.org/job/PreCommit-YARN-Build/8866/artifact/patchprocess/testrun_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/8866/testReport/ Java 1.7.0_55 uname Linux asf901.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/8866/console This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Patch 005. In sync with trunk, use of org.apache.commons.cli complete with all tests passing.

          This patch may not seem directly relevant to people, but it is based on the stable launcher code of Slider, and is designed to achieve one thing: provide a robust launcher for any YARN service. That is, if you are a YARN service, this can run you. If you add the right interface, you get the CLI that came in, and a simple exec() method. If not: no special CLI args, but your config is all set up from the -D key=val and --conf configuration file settings.

          The launcher has lots of error handling, and anything with an exit code (any ExitUtils exception or any other exception that implements ExitCodeProvider, has the exception turned into the exit code of the app. This means a service can return specific failure codes simply by throwing the appropriate exception. Oh, and there's a standard set of exit codes. Based on the slider experience, these approximate the HTTP error codes, so "41" == "401" == unauth; "50" == "500" == service exception. It may seem pointless, but it actually avoids you having to learn some new random exit codes.

          This patch has been around for a while; it's ready to go in. Can someone review it?

          Show
          stevel@apache.org Steve Loughran added a comment - Patch 005. In sync with trunk, use of org.apache.commons.cli complete with all tests passing. This patch may not seem directly relevant to people, but it is based on the stable launcher code of Slider, and is designed to achieve one thing: provide a robust launcher for any YARN service. That is, if you are a YARN service, this can run you. If you add the right interface, you get the CLI that came in, and a simple exec() method. If not: no special CLI args, but your config is all set up from the -D key=val and --conf configuration file settings. The launcher has lots of error handling, and anything with an exit code (any ExitUtils exception or any other exception that implements ExitCodeProvider , has the exception turned into the exit code of the app. This means a service can return specific failure codes simply by throwing the appropriate exception. Oh, and there's a standard set of exit codes. Based on the slider experience, these approximate the HTTP error codes, so "41" == "401" == unauth; "50" == "500" == service exception. It may seem pointless, but it actually avoids you having to learn some new random exit codes. This patch has been around for a while; it's ready to go in. Can someone review it?
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 22 new or modified test files.
          +1 mvninstall 8m 42s trunk passed
          +1 compile 8m 44s trunk passed with JDK v1.8.0_66
          +1 compile 9m 26s trunk passed with JDK v1.7.0_91
          +1 checkstyle 0m 18s trunk passed
          +1 mvnsite 1m 6s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 1m 55s trunk passed
          +1 javadoc 0m 59s trunk passed with JDK v1.8.0_66
          +1 javadoc 1m 7s trunk passed with JDK v1.7.0_91
          +1 mvninstall 1m 39s the patch passed
          +1 compile 8m 41s the patch passed with JDK v1.8.0_66
          -1 javac 14m 36s root-jdk1.8.0_66 with JDK v1.8.0_66 generated 8 new issues (was 731, now 739).
          +1 javac 8m 41s the patch passed
          +1 compile 9m 35s the patch passed with JDK v1.7.0_91
          -1 javac 24m 12s root-jdk1.7.0_91 with JDK v1.7.0_91 generated 8 new issues (was 724, now 732).
          +1 javac 9m 35s the patch passed
          -1 checkstyle 0m 21s Patch generated 183 new checkstyle issues in hadoop-common-project/hadoop-common (total was 138, now 315).
          +1 mvnsite 1m 7s the patch passed
          +1 mvneclipse 0m 15s the patch passed
          -1 whitespace 0m 0s The patch has 85 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 findbugs 2m 11s the patch passed
          +1 javadoc 0m 59s the patch passed with JDK v1.8.0_66
          -1 javadoc 5m 0s hadoop-common-project_hadoop-common-jdk1.7.0_91 with JDK v1.7.0_91 generated 4 new issues (was 13, now 17).
          +1 javadoc 1m 8s the patch passed with JDK v1.7.0_91
          -1 unit 17m 13s hadoop-common in the patch failed with JDK v1.8.0_66.
          -1 unit 8m 0s hadoop-common in the patch failed with JDK v1.7.0_91.
          +1 asflicense 0m 24s Patch does not generate ASF License warnings.
          85m 27s



          Reason Tests
          JDK v1.8.0_66 Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics
          JDK v1.8.0_66 Timed out junit tests org.apache.hadoop.http.TestHttpServerLifecycle
          JDK v1.7.0_91 Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:0ca8df7
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12781247/YARN-679-005.patch
          JIRA Issue YARN-679
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 24f1265e90b8 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / ed18527
          Default Java 1.7.0_91
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_66 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_91
          findbugs v3.0.0
          javac root-jdk1.8.0_66: https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/diff-compile-javac-root-jdk1.8.0_66.txt
          javac root-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/diff-compile-javac-root-jdk1.7.0_91.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/whitespace-eol.txt
          javadoc hadoop-common-project_hadoop-common-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/diff-javadoc-javadoc-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt
          unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt
          JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/10205/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Max memory used 76MB
          Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/10205/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 22 new or modified test files. +1 mvninstall 8m 42s trunk passed +1 compile 8m 44s trunk passed with JDK v1.8.0_66 +1 compile 9m 26s trunk passed with JDK v1.7.0_91 +1 checkstyle 0m 18s trunk passed +1 mvnsite 1m 6s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 55s trunk passed +1 javadoc 0m 59s trunk passed with JDK v1.8.0_66 +1 javadoc 1m 7s trunk passed with JDK v1.7.0_91 +1 mvninstall 1m 39s the patch passed +1 compile 8m 41s the patch passed with JDK v1.8.0_66 -1 javac 14m 36s root-jdk1.8.0_66 with JDK v1.8.0_66 generated 8 new issues (was 731, now 739). +1 javac 8m 41s the patch passed +1 compile 9m 35s the patch passed with JDK v1.7.0_91 -1 javac 24m 12s root-jdk1.7.0_91 with JDK v1.7.0_91 generated 8 new issues (was 724, now 732). +1 javac 9m 35s the patch passed -1 checkstyle 0m 21s Patch generated 183 new checkstyle issues in hadoop-common-project/hadoop-common (total was 138, now 315). +1 mvnsite 1m 7s the patch passed +1 mvneclipse 0m 15s the patch passed -1 whitespace 0m 0s The patch has 85 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 findbugs 2m 11s the patch passed +1 javadoc 0m 59s the patch passed with JDK v1.8.0_66 -1 javadoc 5m 0s hadoop-common-project_hadoop-common-jdk1.7.0_91 with JDK v1.7.0_91 generated 4 new issues (was 13, now 17). +1 javadoc 1m 8s the patch passed with JDK v1.7.0_91 -1 unit 17m 13s hadoop-common in the patch failed with JDK v1.8.0_66. -1 unit 8m 0s hadoop-common in the patch failed with JDK v1.7.0_91. +1 asflicense 0m 24s Patch does not generate ASF License warnings. 85m 27s Reason Tests JDK v1.8.0_66 Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics JDK v1.8.0_66 Timed out junit tests org.apache.hadoop.http.TestHttpServerLifecycle JDK v1.7.0_91 Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics Subsystem Report/Notes Docker Image:yetus/hadoop:0ca8df7 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12781247/YARN-679-005.patch JIRA Issue YARN-679 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 24f1265e90b8 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / ed18527 Default Java 1.7.0_91 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_66 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_91 findbugs v3.0.0 javac root-jdk1.8.0_66: https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/diff-compile-javac-root-jdk1.8.0_66.txt javac root-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/diff-compile-javac-root-jdk1.7.0_91.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/whitespace-eol.txt javadoc hadoop-common-project_hadoop-common-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/diff-javadoc-javadoc-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-YARN-Build/10205/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/10205/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Max memory used 76MB Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org Console output https://builds.apache.org/job/PreCommit-YARN-Build/10205/console This message was automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment - - edited

          GitHub user steveloughran opened a pull request:
          <cut>

          Show
          githubbot ASF GitHub Bot added a comment - - edited GitHub user steveloughran opened a pull request: <cut>
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Patch 006: resync with trunk, checkstyle

          Show
          stevel@apache.org Steve Loughran added a comment - Patch 006: resync with trunk, checkstyle
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Can someone review this patch, thanks

          Show
          stevel@apache.org Steve Loughran added a comment - Can someone review this patch, thanks
          Hide
          djp Junping Du added a comment -

          Hi Steve Loughran, thanks for updating the patch. I will give it a review today.

          Show
          djp Junping Du added a comment - Hi Steve Loughran , thanks for updating the patch. I will give it a review today.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          have you had a chance yet?

          Show
          stevel@apache.org Steve Loughran added a comment - have you had a chance yet?
          Hide
          djp Junping Du added a comment -

          I am general Ok with proposed APIs. But I was waiting for Jenkins report last time for putting on some comments on details. Let's submit patch first.

          Show
          djp Junping Du added a comment - I am general Ok with proposed APIs. But I was waiting for Jenkins report last time for putting on some comments on details. Let's submit patch first.
          Hide
          djp Junping Du added a comment -

          Sounds like Jenkins not get started as last time. Kick off Jenkins manually.

          Show
          djp Junping Du added a comment - Sounds like Jenkins not get started as last time. Kick off Jenkins manually.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          patch 007. This is patch 006 rebased to trunk and resynced in the hope that jenkins will notice

          Show
          stevel@apache.org Steve Loughran added a comment - patch 007. This is patch 006 rebased to trunk and resynced in the hope that jenkins will notice
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I don't know why the patch is failing; I just did a test apply to trunk and it all worked. recreating and Resubmitting with a new version number

          Show
          stevel@apache.org Steve Loughran added a comment - I don't know why the patch is failing; I just did a test apply to trunk and it all worked. recreating and Resubmitting with a new version number
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Patch 008

          Show
          stevel@apache.org Steve Loughran added a comment - Patch 008
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 31s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 53 new or modified test files.
          0 mvndep 0m 32s Maven dependency ordering for branch
          +1 mvninstall 8m 51s trunk passed
          +1 compile 9m 10s trunk passed
          +1 checkstyle 1m 44s trunk passed
          +1 mvnsite 1m 40s trunk passed
          +1 mvneclipse 0m 26s trunk passed
          +1 findbugs 3m 6s trunk passed
          +1 javadoc 1m 41s trunk passed
          0 mvndep 0m 12s Maven dependency ordering for patch
          -1 mvninstall 0m 41s hadoop-common in the patch failed.
          -1 mvninstall 0m 20s hadoop-yarn-common in the patch failed.
          -1 compile 1m 13s root in the patch failed.
          -1 javac 1m 13s root in the patch failed.
          -1 checkstyle 1m 32s root: The patch generated 181 new + 145 unchanged - 9 fixed = 326 total (was 154)
          -1 mvnsite 0m 44s hadoop-common in the patch failed.
          -1 mvnsite 0m 22s hadoop-yarn-common in the patch failed.
          +1 mvneclipse 0m 22s the patch passed
          -1 whitespace 0m 0s The patch has 108 line(s) that end in whitespace. Use git apply --whitespace=fix.
          -1 findbugs 0m 29s hadoop-common in the patch failed.
          -1 findbugs 0m 21s hadoop-yarn-common in the patch failed.
          -1 javadoc 1m 12s hadoop-common-project_hadoop-common generated 9 new + 1 unchanged - 0 fixed = 10 total (was 1)
          -1 javadoc 0m 37s hadoop-yarn-common in the patch failed.
          -1 unit 0m 56s hadoop-common in the patch failed.
          -1 unit 0m 20s hadoop-yarn-common in the patch failed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          38m 20s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:2c91fd8
          JIRA Issue YARN-679
          GITHUB PR https://github.com/apache/hadoop/pull/68
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux c75ae23252b2 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 4a1cedc
          Default Java 1.8.0_91
          findbugs v3.0.0
          mvninstall https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-mvninstall-hadoop-common-project_hadoop-common.txt
          mvninstall https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-mvninstall-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.txt
          compile https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-compile-root.txt
          javac https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-compile-root.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/diff-checkstyle-root.txt
          mvnsite https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-mvnsite-hadoop-common-project_hadoop-common.txt
          mvnsite https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-mvnsite-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/whitespace-eol.txt
          findbugs https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-findbugs-hadoop-common-project_hadoop-common.txt
          findbugs https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.txt
          javadoc https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/diff-javadoc-javadoc-hadoop-common-project_hadoop-common.txt
          javadoc https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/11855/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: .
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/11855/console
          Powered by Apache Yetus 0.3.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 31s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 53 new or modified test files. 0 mvndep 0m 32s Maven dependency ordering for branch +1 mvninstall 8m 51s trunk passed +1 compile 9m 10s trunk passed +1 checkstyle 1m 44s trunk passed +1 mvnsite 1m 40s trunk passed +1 mvneclipse 0m 26s trunk passed +1 findbugs 3m 6s trunk passed +1 javadoc 1m 41s trunk passed 0 mvndep 0m 12s Maven dependency ordering for patch -1 mvninstall 0m 41s hadoop-common in the patch failed. -1 mvninstall 0m 20s hadoop-yarn-common in the patch failed. -1 compile 1m 13s root in the patch failed. -1 javac 1m 13s root in the patch failed. -1 checkstyle 1m 32s root: The patch generated 181 new + 145 unchanged - 9 fixed = 326 total (was 154) -1 mvnsite 0m 44s hadoop-common in the patch failed. -1 mvnsite 0m 22s hadoop-yarn-common in the patch failed. +1 mvneclipse 0m 22s the patch passed -1 whitespace 0m 0s The patch has 108 line(s) that end in whitespace. Use git apply --whitespace=fix. -1 findbugs 0m 29s hadoop-common in the patch failed. -1 findbugs 0m 21s hadoop-yarn-common in the patch failed. -1 javadoc 1m 12s hadoop-common-project_hadoop-common generated 9 new + 1 unchanged - 0 fixed = 10 total (was 1) -1 javadoc 0m 37s hadoop-yarn-common in the patch failed. -1 unit 0m 56s hadoop-common in the patch failed. -1 unit 0m 20s hadoop-yarn-common in the patch failed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 38m 20s Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Issue YARN-679 GITHUB PR https://github.com/apache/hadoop/pull/68 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c75ae23252b2 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 4a1cedc Default Java 1.8.0_91 findbugs v3.0.0 mvninstall https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-mvninstall-hadoop-common-project_hadoop-common.txt mvninstall https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-mvninstall-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.txt compile https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-compile-root.txt javac https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-compile-root.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/diff-checkstyle-root.txt mvnsite https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-mvnsite-hadoop-common-project_hadoop-common.txt mvnsite https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-mvnsite-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/whitespace-eol.txt findbugs https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-findbugs-hadoop-common-project_hadoop-common.txt findbugs https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.txt javadoc https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/diff-javadoc-javadoc-hadoop-common-project_hadoop-common.txt javadoc https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/11855/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/11855/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: . Console output https://builds.apache.org/job/PreCommit-YARN-Build/11855/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          aw Allen Wittenauer added a comment - - edited

          I don't know why the patch is failing; I just did a test apply to trunk and it all worked. recreating and Resubmitting with a new version number

          ..

          <cut>

          Two things:

          1. Once a JIRA references a github pull request, Yetus prioritizes that over any attached files. There probably should be a change to compare date/time stamps but that's a tremendous amount of work and we just haven't gotten to it yet.

          2. Yetus has more and more trouble applying a PR the more and more individual commits there are. This has a lot to do with how the github API presents diff files vs. patch files and git's own ability to apply said files.

          Two options (either one will work):

          1. Squash the commits into a single commit and use the GH PR
          2. Remove the references to the GH PR or at least change the URL enough that Yetus doesn't pick it up. It will then use the attached files.

          Show
          aw Allen Wittenauer added a comment - - edited I don't know why the patch is failing; I just did a test apply to trunk and it all worked. recreating and Resubmitting with a new version number .. <cut> Two things: 1. Once a JIRA references a github pull request, Yetus prioritizes that over any attached files. There probably should be a change to compare date/time stamps but that's a tremendous amount of work and we just haven't gotten to it yet. 2. Yetus has more and more trouble applying a PR the more and more individual commits there are. This has a lot to do with how the github API presents diff files vs. patch files and git's own ability to apply said files. Two options (either one will work): 1. Squash the commits into a single commit and use the GH PR 2. Remove the references to the GH PR or at least change the URL enough that Yetus doesn't pick it up. It will then use the attached files.
          Hide
          githubbot ASF GitHub Bot added a comment - - edited

          Github user steveloughran closed the pull request at:

          <cut>

          Show
          githubbot ASF GitHub Bot added a comment - - edited Github user steveloughran closed the pull request at: <cut>
          Hide
          stevel@apache.org Steve Loughran added a comment -

          closed the PR, see if that will do it

          Show
          stevel@apache.org Steve Loughran added a comment - closed the PR, see if that will do it
          Hide
          aw Allen Wittenauer added a comment -

          yetus does a regex match on the jira issue....

          Show
          aw Allen Wittenauer added a comment - yetus does a regex match on the jira issue....
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Patch 009, synced up with trunk

          HADOOP-13179 broke things, as it made the building of common options private/static, not, as I needed for some things, subclassable. I fixed by making that protected and synchronized on OptionBuilder, which is what everything should do.

          Allen Wittenauer I've edited references to the PR in the JIRA. Will patches now take, or is there some secret DB I need to alter?

          Show
          stevel@apache.org Steve Loughran added a comment - Patch 009, synced up with trunk HADOOP-13179 broke things, as it made the building of common options private/static, not, as I needed for some things, subclassable. I fixed by making that protected and synchronized on OptionBuilder , which is what everything should do. Allen Wittenauer I've edited references to the PR in the JIRA. Will patches now take, or is there some secret DB I need to alter?
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 33s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 1s The patch appears to include 22 new or modified test files.
          +1 mvninstall 7m 35s trunk passed
          +1 compile 7m 10s trunk passed
          +1 checkstyle 0m 24s trunk passed
          +1 mvnsite 0m 57s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 1m 25s trunk passed
          +1 javadoc 0m 48s trunk passed
          +1 mvninstall 0m 43s the patch passed
          +1 compile 7m 17s the patch passed
          -1 javac 7m 17s root generated 8 new + 709 unchanged - 0 fixed = 717 total (was 709)
          -1 checkstyle 0m 26s hadoop-common-project/hadoop-common: The patch generated 143 new + 119 unchanged - 34 fixed = 262 total (was 153)
          +1 mvnsite 0m 55s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          -1 whitespace 0m 0s The patch has 73 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 findbugs 1m 34s the patch passed
          +1 javadoc 0m 48s the patch passed
          +1 unit 7m 17s hadoop-common in the patch passed.
          +1 asflicense 0m 23s The patch does not generate ASF License warnings.
          39m 29s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12818791/YARN-679-009.patch
          JIRA Issue YARN-679
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 1190dfe4fb46 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / fe20494
          Default Java 1.8.0_91
          findbugs v3.0.0
          javac https://builds.apache.org/job/PreCommit-YARN-Build/12368/artifact/patchprocess/diff-compile-javac-root.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/12368/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/12368/artifact/patchprocess/whitespace-eol.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/12368/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/12368/console
          Powered by Apache Yetus 0.3.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 33s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 1s The patch appears to include 22 new or modified test files. +1 mvninstall 7m 35s trunk passed +1 compile 7m 10s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 57s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 25s trunk passed +1 javadoc 0m 48s trunk passed +1 mvninstall 0m 43s the patch passed +1 compile 7m 17s the patch passed -1 javac 7m 17s root generated 8 new + 709 unchanged - 0 fixed = 717 total (was 709) -1 checkstyle 0m 26s hadoop-common-project/hadoop-common: The patch generated 143 new + 119 unchanged - 34 fixed = 262 total (was 153) +1 mvnsite 0m 55s the patch passed +1 mvneclipse 0m 13s the patch passed -1 whitespace 0m 0s The patch has 73 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 findbugs 1m 34s the patch passed +1 javadoc 0m 48s the patch passed +1 unit 7m 17s hadoop-common in the patch passed. +1 asflicense 0m 23s The patch does not generate ASF License warnings. 39m 29s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12818791/YARN-679-009.patch JIRA Issue YARN-679 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 1190dfe4fb46 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / fe20494 Default Java 1.8.0_91 findbugs v3.0.0 javac https://builds.apache.org/job/PreCommit-YARN-Build/12368/artifact/patchprocess/diff-compile-javac-root.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/12368/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/12368/artifact/patchprocess/whitespace-eol.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/12368/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-YARN-Build/12368/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Patch 010, address complaints from checkstyle as far as I consider necessary. Specifically, sometimes it is better if lines do go beyond 80 chars, and you can be a bit less rigorous in test code than in production about accessibility of fields.

          the complaints about IrqHandler using forbidden classes is valid; it's intended to be a single place for this. Ultimately, other uses in the Hadoop code could be replaced with this.

          Show
          stevel@apache.org Steve Loughran added a comment - Patch 010, address complaints from checkstyle as far as I consider necessary. Specifically, sometimes it is better if lines do go beyond 80 chars, and you can be a bit less rigorous in test code than in production about accessibility of fields. the complaints about IrqHandler using forbidden classes is valid; it's intended to be a single place for this. Ultimately, other uses in the Hadoop code could be replaced with this.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 30s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 22 new or modified test files.
          +1 mvninstall 8m 44s trunk passed
          +1 compile 9m 5s trunk passed
          +1 checkstyle 0m 31s trunk passed
          +1 mvnsite 1m 4s trunk passed
          +1 mvneclipse 0m 16s trunk passed
          +1 findbugs 1m 39s trunk passed
          +1 javadoc 0m 55s trunk passed
          +1 mvninstall 0m 51s the patch passed
          +1 compile 8m 35s the patch passed
          -1 javac 8m 35s root generated 8 new + 709 unchanged - 0 fixed = 717 total (was 709)
          -1 checkstyle 0m 29s hadoop-common-project/hadoop-common: The patch generated 42 new + 119 unchanged - 34 fixed = 161 total (was 153)
          +1 mvnsite 1m 6s the patch passed
          +1 mvneclipse 0m 15s the patch passed
          -1 whitespace 0m 0s The patch has 73 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 findbugs 1m 48s the patch passed
          +1 javadoc 0m 54s the patch passed
          -1 unit 22m 54s hadoop-common in the patch failed.
          +1 asflicense 0m 26s The patch does not generate ASF License warnings.
          61m 5s



          Reason Tests
          Timed out junit tests org.apache.hadoop.http.TestHttpServerLifecycle



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12818866/YARN-679-010.patch
          JIRA Issue YARN-679
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 78053710b77c 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / cda0a28
          Default Java 1.8.0_91
          findbugs v3.0.0
          javac https://builds.apache.org/job/PreCommit-YARN-Build/12374/artifact/patchprocess/diff-compile-javac-root.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/12374/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/12374/artifact/patchprocess/whitespace-eol.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/12374/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/12374/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/12374/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/12374/console
          Powered by Apache Yetus 0.3.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 30s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 22 new or modified test files. +1 mvninstall 8m 44s trunk passed +1 compile 9m 5s trunk passed +1 checkstyle 0m 31s trunk passed +1 mvnsite 1m 4s trunk passed +1 mvneclipse 0m 16s trunk passed +1 findbugs 1m 39s trunk passed +1 javadoc 0m 55s trunk passed +1 mvninstall 0m 51s the patch passed +1 compile 8m 35s the patch passed -1 javac 8m 35s root generated 8 new + 709 unchanged - 0 fixed = 717 total (was 709) -1 checkstyle 0m 29s hadoop-common-project/hadoop-common: The patch generated 42 new + 119 unchanged - 34 fixed = 161 total (was 153) +1 mvnsite 1m 6s the patch passed +1 mvneclipse 0m 15s the patch passed -1 whitespace 0m 0s The patch has 73 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 findbugs 1m 48s the patch passed +1 javadoc 0m 54s the patch passed -1 unit 22m 54s hadoop-common in the patch failed. +1 asflicense 0m 26s The patch does not generate ASF License warnings. 61m 5s Reason Tests Timed out junit tests org.apache.hadoop.http.TestHttpServerLifecycle Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12818866/YARN-679-010.patch JIRA Issue YARN-679 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 78053710b77c 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / cda0a28 Default Java 1.8.0_91 findbugs v3.0.0 javac https://builds.apache.org/job/PreCommit-YARN-Build/12374/artifact/patchprocess/diff-compile-javac-root.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/12374/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/12374/artifact/patchprocess/whitespace-eol.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/12374/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/12374/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/12374/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-YARN-Build/12374/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Patch 011; in sync with recent changes to GenericOptionsParser

          Show
          stevel@apache.org Steve Loughran added a comment - Patch 011; in sync with recent changes to GenericOptionsParser
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 22 new or modified test files.
          +1 mvninstall 7m 0s trunk passed
          +1 compile 6m 48s trunk passed
          +1 checkstyle 0m 25s trunk passed
          +1 mvnsite 0m 52s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 18s trunk passed
          +1 javadoc 0m 45s trunk passed
          +1 mvninstall 0m 38s the patch passed
          +1 compile 6m 48s the patch passed
          -1 javac 6m 48s root generated 8 new + 709 unchanged - 0 fixed = 717 total (was 709)
          -1 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 43 new + 112 unchanged - 33 fixed = 155 total (was 145)
          +1 mvnsite 0m 53s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          -1 whitespace 0m 0s The patch has 73 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 findbugs 1m 28s the patch passed
          +1 javadoc 0m 48s the patch passed
          -1 unit 7m 32s hadoop-common in the patch failed.
          +1 asflicense 0m 23s The patch does not generate ASF License warnings.
          37m 34s



          Reason Tests
          Failed junit tests hadoop.ha.TestZKFailoverController



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823719/YARN-679-011.patch
          JIRA Issue YARN-679
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 22c05489ee7f 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 9f29f42
          Default Java 1.8.0_101
          findbugs v3.0.0
          javac https://builds.apache.org/job/PreCommit-YARN-Build/12771/artifact/patchprocess/diff-compile-javac-root.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/12771/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/12771/artifact/patchprocess/whitespace-eol.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/12771/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/12771/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/12771/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/12771/console
          Powered by Apache Yetus 0.3.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 13s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 22 new or modified test files. +1 mvninstall 7m 0s trunk passed +1 compile 6m 48s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 52s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 18s trunk passed +1 javadoc 0m 45s trunk passed +1 mvninstall 0m 38s the patch passed +1 compile 6m 48s the patch passed -1 javac 6m 48s root generated 8 new + 709 unchanged - 0 fixed = 717 total (was 709) -1 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 43 new + 112 unchanged - 33 fixed = 155 total (was 145) +1 mvnsite 0m 53s the patch passed +1 mvneclipse 0m 13s the patch passed -1 whitespace 0m 0s The patch has 73 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 findbugs 1m 28s the patch passed +1 javadoc 0m 48s the patch passed -1 unit 7m 32s hadoop-common in the patch failed. +1 asflicense 0m 23s The patch does not generate ASF License warnings. 37m 34s Reason Tests Failed junit tests hadoop.ha.TestZKFailoverController Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823719/YARN-679-011.patch JIRA Issue YARN-679 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 22c05489ee7f 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 9f29f42 Default Java 1.8.0_101 findbugs v3.0.0 javac https://builds.apache.org/job/PreCommit-YARN-Build/12771/artifact/patchprocess/diff-compile-javac-root.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/12771/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/12771/artifact/patchprocess/whitespace-eol.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/12771/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/12771/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/12771/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-YARN-Build/12771/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          templedf Daniel Templeton added a comment -

          Steve Loughran, I'll take a better look when I get a chance. I just took a quick look and found these two items:

            private ServiceStateException(int exitCode,
                String message,
                Throwable cause) {
               super(message, cause);
          
              this.exitCode = (cause instanceof ExitCodeProvider)?
                              (((ExitCodeProvider) cause).getExitCode())
                             : exitCode;
             }
          

          I think this is backwards. If I'm giving you an exit code, I expect you to use it. We should only default to the cause's code when we aren't given one.

            @Override
            public Configuration bindArgs(Configuration config, List<String> args) throws
                Exception {
              if (LOG.isDebugEnabled()) {
                LOG.debug("Service {} passed in {} arguments:", getName(), args.size());
                for (String arg : args) {
                  LOG.debug(arg);
                }
              }
              return config;
            }
          

          Rather than logging each arg bare and independent, it would be better to either build them into a string and attach it to the main log line, or at the very least log each one with a header or some kind. Just having args spit out with a time stamp and DEBUG in front of them could be confusing.

          I didn't make it very far into the code, so I wouldn't go posting another patch just yet. I'll try to finish the review in the near future.

          Show
          templedf Daniel Templeton added a comment - Steve Loughran , I'll take a better look when I get a chance. I just took a quick look and found these two items: private ServiceStateException( int exitCode, String message, Throwable cause) { super (message, cause); this .exitCode = (cause instanceof ExitCodeProvider)? (((ExitCodeProvider) cause).getExitCode()) : exitCode; } I think this is backwards. If I'm giving you an exit code, I expect you to use it. We should only default to the cause's code when we aren't given one. @Override public Configuration bindArgs(Configuration config, List< String > args) throws Exception { if (LOG.isDebugEnabled()) { LOG.debug( "Service {} passed in {} arguments:" , getName(), args.size()); for ( String arg : args) { LOG.debug(arg); } } return config; } Rather than logging each arg bare and independent, it would be better to either build them into a string and attach it to the main log line, or at the very least log each one with a header or some kind. Just having args spit out with a time stamp and DEBUG in front of them could be confusing. I didn't make it very far into the code, so I wouldn't go posting another patch just yet. I'll try to finish the review in the near future.
          Hide
          templedf Daniel Templeton added a comment -

          BTW, great job on the javadoc.

          Show
          templedf Daniel Templeton added a comment - BTW, great job on the javadoc.
          Hide
          djp Junping Du added a comment -

          Agree with Daniel Templeton that this is well written javadoc that can be good example for reference in future.
          Good to see we finally figure out Jenkins' problem last week. Sorry I haven't figure out too much time this week to review it thoroughly. Just have some level comments here:
          1. It looks like all changes are in hadoop-common project currently. If so, shall we turn it into Hadoop JIRA instead of a YARN issue? Or we think these APIs only get used for YARN application. If so, may be we should consider to refactor most code to under hadoop-yarn-project (like: yarn-api)?

          2. The lifecycle of yarn service looks good to me. However, I haven't see an detail example to show case how an application can use these code to launch yarn service. Do we have a quick dummy application here or we can plan to add one in future.

          3. I saw we were wrapping a lot of error code here around all kinds of exceptions or HTTP code.

          + * <pre>
          + *    0-10: general command issues
          + *   30-39: equivalent to the 3XX responses, where those responses are
          + *          considered errors by the application.
          + *   40-49: client-side/CLI/config problems
          + *   50-59: service-side problems.
          + *   60+  : application specific error codes
          + * </pre>
          

          It sounds like 11-20 is vacant. Is that reserved for other purpose. Also, if error code is used out (like service side problem is more than 10 kinds). How can we extend according to current design?

          I need more time for check more details of the patch which is very huge now.

          Show
          djp Junping Du added a comment - Agree with Daniel Templeton that this is well written javadoc that can be good example for reference in future. Good to see we finally figure out Jenkins' problem last week. Sorry I haven't figure out too much time this week to review it thoroughly. Just have some level comments here: 1. It looks like all changes are in hadoop-common project currently. If so, shall we turn it into Hadoop JIRA instead of a YARN issue? Or we think these APIs only get used for YARN application. If so, may be we should consider to refactor most code to under hadoop-yarn-project (like: yarn-api)? 2. The lifecycle of yarn service looks good to me. However, I haven't see an detail example to show case how an application can use these code to launch yarn service. Do we have a quick dummy application here or we can plan to add one in future. 3. I saw we were wrapping a lot of error code here around all kinds of exceptions or HTTP code. + * <pre> + * 0-10: general command issues + * 30-39: equivalent to the 3XX responses, where those responses are + * considered errors by the application. + * 40-49: client-side/CLI/config problems + * 50-59: service-side problems. + * 60+ : application specific error codes + * </pre> It sounds like 11-20 is vacant. Is that reserved for other purpose. Also, if error code is used out (like service side problem is more than 10 kinds). How can we extend according to current design? I need more time for check more details of the patch which is very huge now.
          Hide
          templedf Daniel Templeton added a comment -

          I made it a bit farther. Additional comments:

          • I see that the pattern of the code from the cause trumping the code parameter shows up in several other places. I see what's you're going for, but I really dislike the idea of passing in two arguments for the same data with one being ignored. I'm open to discuss it.
          • Should the ServiceStateException and ExitUtil.ExitException have a constructor that only takes a message and exit code?
          • Shouldn't need the period after
            * {@inheritDoc}.
          • I'd rather see
              private static final Logger LOG = LoggerFactory.getLogger(
                  HadoopUncaughtExceptionHandler.class);

            break on the equals instead of the paren.

          Here are also some language issues from the package docs:

          • Should have an "and" instead of a comma:
             down the CLI arguments, to execute an operation without having to
          • Should be "out" not "our":
            exit if it times our or a second interrupt is received.
          • I'd rephrase:
            It will instantiate the service classname provided, by zero-args constructor. Or, if none is available, falling back to a constructor that takes a {@code String} as its parameter, on the assumption that the parameter is the service name.

            as

            It will instantiate the service classname provided, using the no-args constructor, or if no such constructor is available, it will fall back to a constructor with a single {@code String} parameter, passing the service name as the parameter value.
          • Missing a close paren before the comma here:
            {@code STATE.STOPPED}, escalated into a non-zero return code
          • Extra space before the dash (and add another hyphen to make it a dash or use ):
            (prepare configuration files -covered later)

            .

          • Same here:
            and control-C signals -calling {@code stop()} on the service when signalled
          • And here:
            parse the command line -it just becomes the responsibility of the
          • And here:
            (prepare configuration files --covered later)
          • Here the hyphen should be a comma:
            and start it -triggering shutdown when signalled
          • Same here:
            It adds two methods to the service interface -and hence two new features

            , though I suppose a dash could make sense...

          • And here:
            returned by the method -so services may signal failures simply by returning
          • And here:
            Exceptions are converted into exit codes -but rather than simply
          • "Signalled" is misspelled several times. Should be "signaled"
          • (there is way covered later)

            is missing an "a"

          • Here:
            commands can be implemented as such services, so integrating with YARN's

            , the "so" should be a "thus"

          • Same here:
            initialized, so allowing services to tune their configuration data before
          • This hyphen is spurious:
            -which may be needed to trigger the injection of HDFS or YARN resources
          • Same here:
            of returning error codes to signal failures -and for 
          • And here:
            these classes -simply to force in the common resources
          • And here:
            files are merged -with the latest file on the command line being the
          • Remove the space before the comma:
            have a "something went wrong" exit code , exceptions <i>may</i>
          • "such" should be "the":
            the service instance and such like
          • Missing an "i" in "is":
            exit code of 0 s created
          • Here the "-so" should just be a comma:
            stop the service if a shutdown request is received -so ensuring that

          Maybe after the weekend I'll be up to finishing the review.

          Show
          templedf Daniel Templeton added a comment - I made it a bit farther. Additional comments: I see that the pattern of the code from the cause trumping the code parameter shows up in several other places. I see what's you're going for, but I really dislike the idea of passing in two arguments for the same data with one being ignored. I'm open to discuss it. Should the ServiceStateException and ExitUtil.ExitException have a constructor that only takes a message and exit code? Shouldn't need the period after * {@inheritDoc}. I'd rather see private static final Logger LOG = LoggerFactory.getLogger( HadoopUncaughtExceptionHandler.class); break on the equals instead of the paren. Here are also some language issues from the package docs: Should have an "and" instead of a comma: down the CLI arguments, to execute an operation without having to Should be "out" not "our": exit if it times our or a second interrupt is received. I'd rephrase: It will instantiate the service classname provided, by zero-args constructor. Or, if none is available, falling back to a constructor that takes a {@code String } as its parameter, on the assumption that the parameter is the service name. as It will instantiate the service classname provided, using the no-args constructor, or if no such constructor is available, it will fall back to a constructor with a single {@code String } parameter, passing the service name as the parameter value. Missing a close paren before the comma here: {@code STATE.STOPPED}, escalated into a non-zero return code Extra space before the dash (and add another hyphen to make it a dash or use — ): (prepare configuration files -covered later) . Same here: and control-C signals -calling {@code stop()} on the service when signalled And here: parse the command line -it just becomes the responsibility of the And here: (prepare configuration files --covered later) Here the hyphen should be a comma: and start it -triggering shutdown when signalled Same here: It adds two methods to the service interface -and hence two new features , though I suppose a dash could make sense... And here: returned by the method -so services may signal failures simply by returning And here: Exceptions are converted into exit codes -but rather than simply "Signalled" is misspelled several times. Should be "signaled" (there is way covered later) is missing an "a" Here: commands can be implemented as such services, so integrating with YARN's , the "so" should be a "thus" Same here: initialized, so allowing services to tune their configuration data before This hyphen is spurious: -which may be needed to trigger the injection of HDFS or YARN resources Same here: of returning error codes to signal failures -and for And here: these classes -simply to force in the common resources And here: files are merged -with the latest file on the command line being the Remove the space before the comma: have a "something went wrong" exit code , exceptions <i>may</i> "such" should be "the": the service instance and such like Missing an "i" in "is": exit code of 0 s created Here the "-so" should just be a comma: stop the service if a shutdown request is received -so ensuring that Maybe after the weekend I'll be up to finishing the review.
          Hide
          templedf Daniel Templeton added a comment -

          Continuing on...

          • I don't see the point of having a shutdown() method in ServiceShutdownHook that is separate from the run() method.
          • ServiceLaunchException appears to be the only exception you've defined that doesn't implement your cause exit code promotion.
          • This hyphen is spurious:
               * to build the formatted exception -in the ENGLISH locale.
          • This line
               * It is still be available as a parameter for the format.

            should be

               * It will also be used as a parameter for the format.
          • In InterruptEscalator here:
                ServiceLauncher owner = ownerRef.get();
                if (owner != null) {
                  sb.append(", owner= ").append(owner.toString());
                }

            it would be nice to also include some note that the owner has already been reaped if it has. Conveying information through omission is seldom clear.

          • In InterruptEscalator.interrupted(), if the service has already been interrupted, it will log the message as a warning twice.
          • In InterruptEscalator.interrupted() here:
                  //wait for that thread to finish
                  try {
                    thread.join(shutdownTimeMillis);
                  } catch (InterruptedException ignored) {
                    //ignored
                  }
                  forcedShutdownTimedOut = !shutdown.getServiceWasShutdown();
                  if (forcedShutdownTimedOut) {
                    LOG.warn("Service did not shut down in time");
                  }

            if the shutdown is interrupted, you will erroneously log that the service did not shut down in time.

          • In InterruptEscalator.interrupted() here:
              /**
               * Previous interrupt handlers. These are not queried.
               */
              private final List<IrqHandler> interruptHandlers = new ArrayList<>(2);

            the comment doesn't make any sense to me based on the code. I also don't see any provision made to deal with multiple handlers per signal. Maybe list list should be a map so you can complain if the handler is already set?

          More later...

          Show
          templedf Daniel Templeton added a comment - Continuing on... I don't see the point of having a shutdown() method in ServiceShutdownHook that is separate from the run() method. ServiceLaunchException appears to be the only exception you've defined that doesn't implement your cause exit code promotion. This hyphen is spurious: * to build the formatted exception -in the ENGLISH locale. This line * It is still be available as a parameter for the format. should be * It will also be used as a parameter for the format. In InterruptEscalator here: ServiceLauncher owner = ownerRef.get(); if (owner != null ) { sb.append( ", owner= " ).append(owner.toString()); } it would be nice to also include some note that the owner has already been reaped if it has. Conveying information through omission is seldom clear. In InterruptEscalator.interrupted() , if the service has already been interrupted, it will log the message as a warning twice. In InterruptEscalator.interrupted() here: //wait for that thread to finish try { thread.join(shutdownTimeMillis); } catch (InterruptedException ignored) { //ignored } forcedShutdownTimedOut = !shutdown.getServiceWasShutdown(); if (forcedShutdownTimedOut) { LOG.warn( "Service did not shut down in time" ); } if the shutdown is interrupted, you will erroneously log that the service did not shut down in time. In InterruptEscalator.interrupted() here: /** * Previous interrupt handlers. These are not queried. */ private final List<IrqHandler> interruptHandlers = new ArrayList<>(2); the comment doesn't make any sense to me based on the code. I also don't see any provision made to deal with multiple handlers per signal. Maybe list list should be a map so you can complain if the handler is already set? More later...
          Hide
          templedf Daniel Templeton added a comment -

          Continuing:

          • The hyphen should be "that":
             * Handler of interrupts -relays them to a registered
          • This:
               * Run a service -called after {@link Service#start()}.

            should be

            +   * Run a service. This method is called after {@link Service#start()}.
          • The period should be a colon here:
               *   <li>Any other exception. A new {@link ServiceLaunchException} is created
          • This hyphen should be a dash or a comma:
             * the GenericOptionsParser -simply extracted to constants.
          • Same here:
               * except in the case that the class does load but it isn't actually
          • The @value }} tags in the {{LauncherArguments.ARG_CONFCLASS and LauncherArguments. E_PARSE_FAILED javadocs are just kinda dangling out there, not really adding anything--except maybe confusion.
          • The javadoc here:
            LauncherExitCodes.EXIT_FAIL

            doesn't match the pattern for the rest of the class' constants.

          • The 40x/50x comments in the LauncherExitCodes class' constants' javadoc need a little context around them. Otherwise it just seems like technical Tourettes.
          • The hyphen here should be a dash:
             *   <li>If any exception is raised and provides an exit code
             *   -that is, it implements {@link ExitCodeProvider},
          • "configurations" should be possessive, not plural:
             * is wrong and logger configurations not on it, then no error messages by
          • In ServiceLauncher.launchServiceAndExit(), this bit
                for (String arg : args) {
                  builder.append('"').append(arg).append("\" ");
                }

            could be pulling out into another method and run lazily since it's only needed in exceptional cases. You could also reuse it in parseCommandArgs().

          • The first sentence of a javadoc header should summarize the content. This one doesn't:
              /**
               * An exception has been raised.
               * Save it to {@link #serviceException}, with the exit code in
               * {@link #serviceExitCode}
               * @param exitException exception
               */
          • Remove the period:
               * @return the new options.
          • The hyphen here should be a dash or "because":
                          + "- it is not a Configuration class/subclass");
          • The hyphen here should be a period or semicolon:
                    LOG.debug("Failed to load {} -it is not on the classpath", classname);
          • "LaunchedService" should be "LaunchableService":
                  // it's a launchedService, pass in the conf and arguments before init)
                  LOG.debug("Service {} implements LaunchedService", name);
                  launchableService = (LaunchableService) service;
                  if (launchableService.isInState(Service.STATE.INITED)) {
                    LOG.warn("LaunchedService {}" 
            
          • Won't this lead to confusing exceptions with stack traces as their messages followed by their own stack traces?
                  if (message == null) {
                    // some exceptions do not have a message; fall back
                    // to the string value.
                    message = thrown.toString();
                  }
          • The javadoc for ServiceLauncher. registerFailureHandling() omits that it also registers a handler for SIGTERM.
          • The "Override point:" note seems to be missing from many of the ServiceLauncher methods' javadoc.
          • This should be in the check to see if logging is enabled:
                LOG.debug("Command line: {}", argString);

          I have to be honest; my eyes glazed over by the end, and I'm sure the quality of my review suffered. The only thing that kept me going was the comforting thought that you'll have as much fun sorting through my litany of comments as I did digging through all that code.

          Would it be possible to break this down into a few smaller patches? It would help tremendously in getting it reviewed.

          Show
          templedf Daniel Templeton added a comment - Continuing: The hyphen should be "that": * Handler of interrupts -relays them to a registered This: * Run a service -called after {@link Service#start()}. should be + * Run a service. This method is called after {@link Service#start()}. The period should be a colon here: * <li>Any other exception. A new {@link ServiceLaunchException} is created This hyphen should be a dash or a comma: * the GenericOptionsParser -simply extracted to constants. Same here: * except in the case that the class does load but it isn't actually The @value }} tags in the {{LauncherArguments.ARG_CONFCLASS and LauncherArguments. E_PARSE_FAILED javadocs are just kinda dangling out there, not really adding anything--except maybe confusion. The javadoc here: LauncherExitCodes.EXIT_FAIL doesn't match the pattern for the rest of the class' constants. The 40x/50x comments in the LauncherExitCodes class' constants' javadoc need a little context around them. Otherwise it just seems like technical Tourettes. The hyphen here should be a dash: * <li>If any exception is raised and provides an exit code * -that is, it implements {@link ExitCodeProvider}, "configurations" should be possessive, not plural: * is wrong and logger configurations not on it, then no error messages by In ServiceLauncher.launchServiceAndExit() , this bit for ( String arg : args) { builder.append(' "').append(arg).append(" \ " " ); } could be pulling out into another method and run lazily since it's only needed in exceptional cases. You could also reuse it in parseCommandArgs() . The first sentence of a javadoc header should summarize the content. This one doesn't: /** * An exception has been raised. * Save it to {@link #serviceException}, with the exit code in * {@link #serviceExitCode} * @param exitException exception */ Remove the period: * @ return the new options. The hyphen here should be a dash or "because": + "- it is not a Configuration class/subclass" ); The hyphen here should be a period or semicolon: LOG.debug( "Failed to load {} -it is not on the classpath" , classname); "LaunchedService" should be "LaunchableService": // it's a launchedService, pass in the conf and arguments before init) LOG.debug( "Service {} implements LaunchedService" , name); launchableService = (LaunchableService) service; if (launchableService.isInState(Service.STATE.INITED)) { LOG.warn( "LaunchedService {}" Won't this lead to confusing exceptions with stack traces as their messages followed by their own stack traces? if (message == null ) { // some exceptions do not have a message; fall back // to the string value. message = thrown.toString(); } The javadoc for ServiceLauncher. registerFailureHandling() omits that it also registers a handler for SIGTERM. The "Override point:" note seems to be missing from many of the ServiceLauncher methods' javadoc. This should be in the check to see if logging is enabled: LOG.debug( "Command line: {}" , argString); I have to be honest; my eyes glazed over by the end, and I'm sure the quality of my review suffered. The only thing that kept me going was the comforting thought that you'll have as much fun sorting through my litany of comments as I did digging through all that code. Would it be possible to break this down into a few smaller patches? It would help tremendously in getting it reviewed.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Dan, I've broken things down all that happens iis parts of the patch get ignored. Well, ignored more than this. What we have here is self-contained, and, being derived with what we've been using in Slider, not far off what's been used elsewhere. So afraid not.

          but: thank you very, very much for the reviews, I'm going take a break from s3 coding to address them

          -steve

          Show
          stevel@apache.org Steve Loughran added a comment - Dan, I've broken things down all that happens iis parts of the patch get ignored. Well, ignored more than this. What we have here is self-contained, and, being derived with what we've been using in Slider, not far off what's been used elsewhere. So afraid not. but: thank you very, very much for the reviews, I'm going take a break from s3 coding to address them -steve

            People

            • Assignee:
              stevel@apache.org Steve Loughran
              Reporter:
              stevel@apache.org Steve Loughran
            • Votes:
              0 Vote for this issue
              Watchers:
              25 Start watching this issue

              Dates

              • Created:
                Updated:

                Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0h
                0h
                Logged:
                Time Spent - 72h
                72h

                  Development