Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-14637

GenericTestUtils.waitFor needs to check condition again after max wait time

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0-alpha4
    • Fix Version/s: 2.9.0, 3.0.0-beta1
    • Component/s: None
    • Labels:
    • Hadoop Flags:
      Reviewed

      Description

      org.apache.hadoop.yarn.server.resourcemanager.TestRMRestart
      testRMRestartWaitForPreviousAMToFinish(org.apache.hadoop.yarn.server.resourcemanager.TestRMRestart)  Time elapsed: 23.718 sec  <<< ERROR!
      java.lang.IllegalArgumentException: Total wait time should be greater than check interval time
      	at com.google.common.base.Preconditions.checkArgument(Preconditions.java:88)
      	at org.apache.hadoop.test.GenericTestUtils.waitFor(GenericTestUtils.java:341)
      	at org.apache.hadoop.yarn.server.resourcemanager.TestRMRestart.testRMRestartWaitForPreviousAMToFinish(TestRMRestart.java:618)
      
      1. HADOOP-14637.001.patch
        1 kB
        Daniel Templeton
      2. HADOOP-14637.002.patch
        2 kB
        Daniel Templeton
      3. HADOOP-14637.003.patch
        5 kB
        Daniel Templeton
      4. HADOOP-14637.004.patch
        4 kB
        Daniel Templeton
      5. HADOOP-14637.005.patch
        2 kB
        Daniel Templeton

        Issue Links

          Activity

          Hide
          templedf Daniel Templeton added a comment -

          The test if the wait time is less than the interval is superfluous unless we're going to enforce that the wait time is a multiple of the interval.

          Show
          templedf Daniel Templeton added a comment - The test if the wait time is less than the interval is superfluous unless we're going to enforce that the wait time is a multiple of the interval.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 10s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          +1 mvninstall 13m 6s trunk passed
          +1 compile 13m 30s trunk passed
          +1 checkstyle 0m 38s trunk passed
          +1 mvnsite 1m 31s trunk passed
          +1 findbugs 1m 22s trunk passed
          +1 javadoc 0m 48s trunk passed
                Patch Compile Tests
          +1 mvninstall 0m 38s the patch passed
          +1 compile 10m 5s the patch passed
          +1 javac 10m 5s the patch passed
          +1 checkstyle 0m 35s the patch passed
          +1 mvnsite 1m 24s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 29s the patch passed
          +1 javadoc 0m 50s the patch passed
                Other Tests
          -1 unit 7m 59s hadoop-common in the patch failed.
          +1 asflicense 0m 34s The patch does not generate ASF License warnings.
          56m 29s



          Reason Tests
          Failed junit tests hadoop.security.TestKDiag
            hadoop.test.TestGenericTestUtils



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14637
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12876340/HADOOP-14637.001.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 9ae73923920b 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 3de47ab
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12747/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12747/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12747/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 10s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 13m 6s trunk passed +1 compile 13m 30s trunk passed +1 checkstyle 0m 38s trunk passed +1 mvnsite 1m 31s trunk passed +1 findbugs 1m 22s trunk passed +1 javadoc 0m 48s trunk passed       Patch Compile Tests +1 mvninstall 0m 38s the patch passed +1 compile 10m 5s the patch passed +1 javac 10m 5s the patch passed +1 checkstyle 0m 35s the patch passed +1 mvnsite 1m 24s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 29s the patch passed +1 javadoc 0m 50s the patch passed       Other Tests -1 unit 7m 59s hadoop-common in the patch failed. +1 asflicense 0m 34s The patch does not generate ASF License warnings. 56m 29s Reason Tests Failed junit tests hadoop.security.TestKDiag   hadoop.test.TestGenericTestUtils Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14637 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12876340/HADOOP-14637.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 9ae73923920b 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 3de47ab Default Java 1.8.0_131 findbugs v3.1.0-RC1 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12747/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12747/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12747/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          templedf Daniel Templeton added a comment -

          Fixed related unit test failure.

          Show
          templedf Daniel Templeton added a comment - Fixed related unit test failure.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 11s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
                trunk Compile Tests
          +1 mvninstall 13m 12s trunk passed
          +1 compile 13m 27s trunk passed
          +1 checkstyle 0m 34s trunk passed
          +1 mvnsite 1m 26s trunk passed
          +1 findbugs 1m 23s trunk passed
          +1 javadoc 0m 48s trunk passed
                Patch Compile Tests
          +1 mvninstall 0m 39s the patch passed
          +1 compile 10m 2s the patch passed
          +1 javac 10m 2s the patch passed
          -0 checkstyle 0m 36s hadoop-common-project/hadoop-common: The patch generated 1 new + 18 unchanged - 0 fixed = 19 total (was 18)
          +1 mvnsite 1m 26s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 30s the patch passed
          +1 javadoc 0m 50s the patch passed
                Other Tests
          -1 unit 8m 0s hadoop-common in the patch failed.
          +1 asflicense 0m 33s The patch does not generate ASF License warnings.
          56m 26s



          Reason Tests
          Failed junit tests hadoop.fs.sftp.TestSFTPFileSystem
            hadoop.security.TestKDiag



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14637
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12876351/HADOOP-14637.002.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux a045b7689585 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 3de47ab
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12748/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12748/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12748/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12748/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 11s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.       trunk Compile Tests +1 mvninstall 13m 12s trunk passed +1 compile 13m 27s trunk passed +1 checkstyle 0m 34s trunk passed +1 mvnsite 1m 26s trunk passed +1 findbugs 1m 23s trunk passed +1 javadoc 0m 48s trunk passed       Patch Compile Tests +1 mvninstall 0m 39s the patch passed +1 compile 10m 2s the patch passed +1 javac 10m 2s the patch passed -0 checkstyle 0m 36s hadoop-common-project/hadoop-common: The patch generated 1 new + 18 unchanged - 0 fixed = 19 total (was 18) +1 mvnsite 1m 26s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 30s the patch passed +1 javadoc 0m 50s the patch passed       Other Tests -1 unit 8m 0s hadoop-common in the patch failed. +1 asflicense 0m 33s The patch does not generate ASF License warnings. 56m 26s Reason Tests Failed junit tests hadoop.fs.sftp.TestSFTPFileSystem   hadoop.security.TestKDiag Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14637 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12876351/HADOOP-14637.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a045b7689585 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 3de47ab Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12748/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12748/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12748/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12748/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          templedf Daniel Templeton added a comment -

          Unit test failures are unrelated.

          Show
          templedf Daniel Templeton added a comment - Unit test failures are unrelated.
          Hide
          jlowe Jason Lowe added a comment -

          I think the precondition check really did catch a bug here. From the TestRMRestart code:

              final int maxRetry = 10;
              final RMApp rmAppForCheck = rmApp;
              GenericTestUtils.waitFor(
                  new Supplier<Boolean>() {
                    @Override
                    public Boolean get() {
                      return new Boolean(rmAppForCheck.getAppAttempts().size() == 4);
                    }
                  },
                  100, maxRetry);
              Assert.assertEquals(RMAppAttemptState.FAILED,
                  rmApp.getAppAttempts().get(latestAppAttemptId).getAppAttemptState());
          

          From the variable names and values, I'm guessing the intent here was to check every 100 milliseconds for 10 total checks (i.e.: a maximum cumulative wait time of one second). However this code is only going to check once, and if that check fails it will sleep for 100 milliseconds then throw an exception. That is clearly not intended by the caller, otherwise they would skip all this boilerplate and just code up the check directly in their unit test.

          Besides the bug in TestRMRestart, it would be useful for GenericTestUtils.waitFor to do one last check after the time expired before throwing the timeout exception. At least that would do something semantically useful if we remove this precondition check and allow the wait interval to be less than the check interval.

          Show
          jlowe Jason Lowe added a comment - I think the precondition check really did catch a bug here. From the TestRMRestart code: final int maxRetry = 10; final RMApp rmAppForCheck = rmApp; GenericTestUtils.waitFor( new Supplier< Boolean >() { @Override public Boolean get() { return new Boolean (rmAppForCheck.getAppAttempts().size() == 4); } }, 100, maxRetry); Assert.assertEquals(RMAppAttemptState.FAILED, rmApp.getAppAttempts().get(latestAppAttemptId).getAppAttemptState()); From the variable names and values, I'm guessing the intent here was to check every 100 milliseconds for 10 total checks (i.e.: a maximum cumulative wait time of one second). However this code is only going to check once, and if that check fails it will sleep for 100 milliseconds then throw an exception. That is clearly not intended by the caller, otherwise they would skip all this boilerplate and just code up the check directly in their unit test. Besides the bug in TestRMRestart, it would be useful for GenericTestUtils.waitFor to do one last check after the time expired before throwing the timeout exception. At least that would do something semantically useful if we remove this precondition check and allow the wait interval to be less than the check interval.
          Hide
          templedf Daniel Templeton added a comment -

          Yeah, I missed the actual bug. My point was that the precondition is throwing an exception if the wait time is less than the interval, but that only makes sense if the wait time is a multiple of the interval. The scenario you described is the same in any case where that's not true. Ex: wait=110, interval=100 => try, sleep, try, sleep, throw exception. The final test before giving up is required regardless of whether the precondition is removed.

          Show
          templedf Daniel Templeton added a comment - Yeah, I missed the actual bug. My point was that the precondition is throwing an exception if the wait time is less than the interval, but that only makes sense if the wait time is a multiple of the interval. The scenario you described is the same in any case where that's not true. Ex: wait=110, interval=100 => try, sleep, try, sleep, throw exception. The final test before giving up is required regardless of whether the precondition is removed.
          Hide
          jlowe Jason Lowe added a comment -

          The check after looping is always needed, even for the case where wait time is a multiple of the check interval. We should not assume the check call is instantaneous nor that we wakeup exactly when requested. So sounds like there's two things to fix here, the missing check after the wait expires and fixing the TestRMRestart case to call with correct arguments. I'm hesitant to remove the precondition check, and it caught a bug here. Is there a real need to allow wait < check in practice?

          Show
          jlowe Jason Lowe added a comment - The check after looping is always needed, even for the case where wait time is a multiple of the check interval. We should not assume the check call is instantaneous nor that we wakeup exactly when requested. So sounds like there's two things to fix here, the missing check after the wait expires and fixing the TestRMRestart case to call with correct arguments. I'm hesitant to remove the precondition check, and it caught a bug here. Is there a real need to allow wait < check in practice?
          Hide
          templedf Daniel Templeton added a comment - - edited

          I was just having exactly that internal debate. If the call were being made programmatically, i.e. with generated wait time and internal, having the check could cause an unexpected failure. Not really a strong argument, but I can't come up with any compelling reason to keep the check. The reason to keep it would be to catch the case where the caller gets confused and thinks the wait time is the number of retries, but that seems above and beyond for an API. As long as it's clearly documented, it's on the caller not to be dumb. I don't know. If you feel strongly either way, consider me swayed.

          Show
          templedf Daniel Templeton added a comment - - edited I was just having exactly that internal debate. If the call were being made programmatically, i.e. with generated wait time and internal, having the check could cause an unexpected failure. Not really a strong argument, but I can't come up with any compelling reason to keep the check. The reason to keep it would be to catch the case where the caller gets confused and thinks the wait time is the number of retries, but that seems above and beyond for an API. As long as it's clearly documented, it's on the caller not to be dumb. I don't know. If you feel strongly either way, consider me swayed.
          Hide
          templedf Daniel Templeton added a comment - - edited

          Here's a patch with the check removed. (003)

          Show
          templedf Daniel Templeton added a comment - - edited Here's a patch with the check removed. (003)
          Hide
          templedf Daniel Templeton added a comment - - edited

          And here's one without removing the check. (004)

          Show
          templedf Daniel Templeton added a comment - - edited And here's one without removing the check. (004)
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
                trunk Compile Tests
          0 mvndep 0m 15s Maven dependency ordering for branch
          +1 mvninstall 14m 55s trunk passed
          +1 compile 16m 9s trunk passed
          +1 checkstyle 2m 4s trunk passed
          +1 mvnsite 2m 26s trunk passed
          +1 findbugs 3m 0s trunk passed
          +1 javadoc 1m 30s trunk passed
                Patch Compile Tests
          0 mvndep 0m 20s Maven dependency ordering for patch
          +1 mvninstall 1m 29s the patch passed
          +1 compile 12m 22s the patch passed
          +1 javac 12m 22s the patch passed
          +1 checkstyle 2m 14s the patch passed
          +1 mvnsite 2m 32s the patch passed
          +1 whitespace 0m 1s The patch has no whitespace issues.
          +1 findbugs 2m 54s the patch passed
          +1 javadoc 1m 27s the patch passed
                Other Tests
          -1 unit 8m 8s hadoop-common in the patch failed.
          -1 unit 42m 37s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 39s The patch does not generate ASF License warnings.
          137m 1s



          Reason Tests
          Failed junit tests hadoop.security.TestKDiag
            hadoop.yarn.server.resourcemanager.TestRMRestart



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14637
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12876511/HADOOP-14637.004.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 4b53dce5269f 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 5496a34
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12757/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12757/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12757/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12757/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.       trunk Compile Tests 0 mvndep 0m 15s Maven dependency ordering for branch +1 mvninstall 14m 55s trunk passed +1 compile 16m 9s trunk passed +1 checkstyle 2m 4s trunk passed +1 mvnsite 2m 26s trunk passed +1 findbugs 3m 0s trunk passed +1 javadoc 1m 30s trunk passed       Patch Compile Tests 0 mvndep 0m 20s Maven dependency ordering for patch +1 mvninstall 1m 29s the patch passed +1 compile 12m 22s the patch passed +1 javac 12m 22s the patch passed +1 checkstyle 2m 14s the patch passed +1 mvnsite 2m 32s the patch passed +1 whitespace 0m 1s The patch has no whitespace issues. +1 findbugs 2m 54s the patch passed +1 javadoc 1m 27s the patch passed       Other Tests -1 unit 8m 8s hadoop-common in the patch failed. -1 unit 42m 37s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 39s The patch does not generate ASF License warnings. 137m 1s Reason Tests Failed junit tests hadoop.security.TestKDiag   hadoop.yarn.server.resourcemanager.TestRMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14637 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12876511/HADOOP-14637.004.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 4b53dce5269f 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 5496a34 Default Java 1.8.0_131 findbugs v3.1.0-RC1 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12757/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12757/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12757/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12757/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jlowe Jason Lowe added a comment -

          Thanks for updating the patch! I'm also torn on whether to keep the arg check or not, but I lean towards keeping it simply for the fact that it caught this bug and would have caught a bug in HDFS-11711 which prompted the change in HADOOP-14568. I believe the net effect is it helps more than it hurts, but like you I don't feel very strongly about it.

          While looking over the latest patch I noticed another small bug that would be nice to fix since we're here. GenericTestUtils.waitFor should be calling Time.monotonicNow rather than Time.now so in case the system clock adjusts during unit testing we don't get an incorrect timeout period. It's not technically related to this change, so if you think it should be addressed in another JIRA I'm good with that too.

          Show
          jlowe Jason Lowe added a comment - Thanks for updating the patch! I'm also torn on whether to keep the arg check or not, but I lean towards keeping it simply for the fact that it caught this bug and would have caught a bug in HDFS-11711 which prompted the change in HADOOP-14568 . I believe the net effect is it helps more than it hurts, but like you I don't feel very strongly about it. While looking over the latest patch I noticed another small bug that would be nice to fix since we're here. GenericTestUtils.waitFor should be calling Time.monotonicNow rather than Time.now so in case the system clock adjusts during unit testing we don't get an incorrect timeout period. It's not technically related to this change, so if you think it should be addressed in another JIRA I'm good with that too.
          Hide
          jlowe Jason Lowe added a comment -

          Updating the summary since YARN-6759 has fixed the unit test in YARN, so this JIRA just focuses on fixing the issues in GenericTestUtils.waitFor.

          Show
          jlowe Jason Lowe added a comment - Updating the summary since YARN-6759 has fixed the unit test in YARN, so this JIRA just focuses on fixing the issues in GenericTestUtils.waitFor.
          Hide
          templedf Daniel Templeton added a comment -

          Here's a new patch that leaves in the check and documents the behavior.

          Show
          templedf Daniel Templeton added a comment - Here's a new patch that leaves in the check and documents the behavior.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          +1 mvninstall 16m 24s trunk passed
          +1 compile 17m 14s trunk passed
          +1 checkstyle 0m 42s trunk passed
          +1 mvnsite 1m 42s trunk passed
          +1 findbugs 1m 47s trunk passed
          +1 javadoc 0m 58s trunk passed
                Patch Compile Tests
          +1 mvninstall 0m 50s the patch passed
          +1 compile 13m 23s the patch passed
          +1 javac 13m 23s the patch passed
          +1 checkstyle 0m 42s the patch passed
          +1 mvnsite 1m 45s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 56s the patch passed
          +1 javadoc 0m 58s the patch passed
                Other Tests
          +1 unit 9m 28s hadoop-common in the patch passed.
          +1 asflicense 0m 40s The patch does not generate ASF License warnings.
          70m 43s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14637
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12877841/HADOOP-14637.005.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 69fb7e31e8d9 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 0b7afc0
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12812/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12812/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 14s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 16m 24s trunk passed +1 compile 17m 14s trunk passed +1 checkstyle 0m 42s trunk passed +1 mvnsite 1m 42s trunk passed +1 findbugs 1m 47s trunk passed +1 javadoc 0m 58s trunk passed       Patch Compile Tests +1 mvninstall 0m 50s the patch passed +1 compile 13m 23s the patch passed +1 javac 13m 23s the patch passed +1 checkstyle 0m 42s the patch passed +1 mvnsite 1m 45s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 56s the patch passed +1 javadoc 0m 58s the patch passed       Other Tests +1 unit 9m 28s hadoop-common in the patch passed. +1 asflicense 0m 40s The patch does not generate ASF License warnings. 70m 43s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14637 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12877841/HADOOP-14637.005.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 69fb7e31e8d9 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 0b7afc0 Default Java 1.8.0_131 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12812/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12812/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jlowe Jason Lowe added a comment -

          Thanks for updating the patch! +1 lgtm, committing this. I'll file a separate JIRA for the monotonicNow issue.

          Show
          jlowe Jason Lowe added a comment - Thanks for updating the patch! +1 lgtm, committing this. I'll file a separate JIRA for the monotonicNow issue.
          Hide
          templedf Daniel Templeton added a comment -

          Oh, whoops. Forgot to address the monotonic time issue. I can recut the patch if you haven't committed it yet.

          Show
          templedf Daniel Templeton added a comment - Oh, whoops. Forgot to address the monotonic time issue. I can recut the patch if you haven't committed it yet.
          Hide
          jlowe Jason Lowe added a comment -

          Thanks, Daniel! I committed this to trunk and branch-2.

          I can recut the patch if you haven't committed it yet.

          No worries, I'll just file the JIRA since it's technically a separate issue anyway. I'll link it to this one, and feel free to put up the two-line change patch for it. If not I'll post something on it probably by next week.

          Show
          jlowe Jason Lowe added a comment - Thanks, Daniel! I committed this to trunk and branch-2. I can recut the patch if you haven't committed it yet. No worries, I'll just file the JIRA since it's technically a separate issue anyway. I'll link it to this one, and feel free to put up the two-line change patch for it. If not I'll post something on it probably by next week.

            People

            • Assignee:
              templedf Daniel Templeton
              Reporter:
              templedf Daniel Templeton
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development