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

Make all MockRM#waitForState consistent.

    Details

      Description

      There are some inconsistencies among these waitForState in MockRM:
      1. Some waitForState return a boolean while others don't.
      2. Some waitForState don't have a timeout, they can wait for ever.
      3. Some waitForState use LOG.info and others use System.out.println to print messages.

      1. YARN-4907.001.patch
        7 kB
        Yufei Gu
      2. YARN-4907.002.patch
        7 kB
        Yufei Gu

        Issue Links

          Activity

          Hide
          yufeigu Yufei Gu added a comment - - edited

          Some waitForState return a boolean since the caller need to assert on different value. Some waitForState doesn't since the caller expect them to assert if the state is reached before timeout. So it makes sense to have both.
          I changed all System.out.println to LOG.info and make sure every wait function has a timeout.

          Show
          yufeigu Yufei Gu added a comment - - edited Some waitForState return a boolean since the caller need to assert on different value. Some waitForState doesn't since the caller expect them to assert if the state is reached before timeout. So it makes sense to have both. I changed all System.out.println to LOG.info and make sure every wait function has a timeout.
          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 1 new or modified test files.
          +1 mvninstall 8m 22s trunk passed
          +1 compile 0m 41s trunk passed
          +1 checkstyle 0m 26s trunk passed
          +1 mvnsite 0m 45s trunk passed
          +1 mvneclipse 0m 16s trunk passed
          +1 findbugs 1m 18s trunk passed
          +1 javadoc 0m 26s trunk passed
          +1 mvninstall 0m 36s the patch passed
          +1 compile 0m 32s the patch passed
          +1 javac 0m 32s the patch passed
          +1 checkstyle 0m 22s the patch passed
          +1 mvnsite 0m 40s the patch passed
          +1 mvneclipse 0m 17s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 19s the patch passed
          +1 javadoc 0m 18s the patch passed
          +1 unit 34m 39s hadoop-yarn-server-resourcemanager in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          52m 10s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829967/YARN-4907.001.patch
          JIRA Issue YARN-4907
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 6e2bc1e4eda5 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / d85d9b2
          Default Java 1.8.0_101
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13195/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13195/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 1 new or modified test files. +1 mvninstall 8m 22s trunk passed +1 compile 0m 41s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 0m 45s trunk passed +1 mvneclipse 0m 16s trunk passed +1 findbugs 1m 18s trunk passed +1 javadoc 0m 26s trunk passed +1 mvninstall 0m 36s the patch passed +1 compile 0m 32s the patch passed +1 javac 0m 32s the patch passed +1 checkstyle 0m 22s the patch passed +1 mvnsite 0m 40s the patch passed +1 mvneclipse 0m 17s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 19s the patch passed +1 javadoc 0m 18s the patch passed +1 unit 34m 39s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 52m 10s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829967/YARN-4907.001.patch JIRA Issue YARN-4907 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 6e2bc1e4eda5 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / d85d9b2 Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13195/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13195/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          miklos.szegedi@cloudera.com Miklos Szegedi added a comment -

          +1 (non-binding) I verified and it looks good to me. Thank you, Yufei Gu!

          You might want to change the wait logic in BaseContainerManagerTest.waitForNMContainerState as well.

          Show
          miklos.szegedi@cloudera.com Miklos Szegedi added a comment - +1 (non-binding) I verified and it looks good to me. Thank you, Yufei Gu ! You might want to change the wait logic in BaseContainerManagerTest.waitForNMContainerState as well.
          Hide
          yufeigu Yufei Gu added a comment -

          Thanks Miklos Szegedi for the review. I will upload a new patch including the case in BaseContainerManagerTest.waitForNMContainerState.

          Show
          yufeigu Yufei Gu added a comment - Thanks Miklos Szegedi for the review. I will upload a new patch including the case in BaseContainerManagerTest.waitForNMContainerState.
          Hide
          yufeigu Yufei Gu added a comment -

          Hi Miklos Szegedi, after looking at the BaseContainerManagerTest.waitForNMContainerState closely, I think better to put it in another JIRA.

          Show
          yufeigu Yufei Gu added a comment - Hi Miklos Szegedi , after looking at the BaseContainerManagerTest.waitForNMContainerState closely, I think better to put it in another JIRA.
          Hide
          templedf Daniel Templeton added a comment -

          Thanks, Yufei Gu. One comment: your while-if-break statements can be simplified. For example:

               while (true) {
                if (timeWaiting >= TIMEOUT_MS_FOR_CONTAINER_AND_NODE) {
                  break;
                }
          

          should be

               while (timeWaiting < TIMEOUT_MS_FOR_CONTAINER_AND_NODE) {
          

          There's another in waitForNewAMToLaunchAndRegister().

          Show
          templedf Daniel Templeton added a comment - Thanks, Yufei Gu . One comment: your while-if-break statements can be simplified. For example: while ( true ) { if (timeWaiting >= TIMEOUT_MS_FOR_CONTAINER_AND_NODE) { break ; } should be while (timeWaiting < TIMEOUT_MS_FOR_CONTAINER_AND_NODE) { There's another in waitForNewAMToLaunchAndRegister() .
          Hide
          yufeigu Yufei Gu added a comment -

          Thanks Daniel Templeton for the review. I've uploaded patch 002 to solve the comment.
          For the one in waitForNewAMToLaunchAndRegister, you give a nice suggestion in YARN-4807 https://issues.apache.org/jira/browse/YARN-4807?focusedCommentId=15246217&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15246217. So I think it's OK to leave it alone.

          Show
          yufeigu Yufei Gu added a comment - Thanks Daniel Templeton for the review. I've uploaded patch 002 to solve the comment. For the one in waitForNewAMToLaunchAndRegister , you give a nice suggestion in YARN-4807 https://issues.apache.org/jira/browse/YARN-4807?focusedCommentId=15246217&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15246217 . So I think it's OK to leave it alone.
          Hide
          templedf Daniel Templeton added a comment -

          Can we simplify this one as well?

              int timeWaiting = 0;
              while (app.getAppAttempts().size() != attemptSize) {
                if (timeWaiting >= TIMEOUT_MS_FOR_ATTEMPT) {
                  break;
                }
          
          Show
          templedf Daniel Templeton added a comment - Can we simplify this one as well? int timeWaiting = 0; while (app.getAppAttempts().size() != attemptSize) { if (timeWaiting >= TIMEOUT_MS_FOR_ATTEMPT) { break ; }
          Hide
          templedf Daniel Templeton added a comment -

          OK, on grounds that I have previously recommended exactly this structure before (e.g. YARN-4807), nevermind.

          Show
          templedf Daniel Templeton added a comment - OK, on grounds that I have previously recommended exactly this structure before (e.g. YARN-4807 ), nevermind.
          Hide
          templedf Daniel Templeton added a comment -

          +1 I'll commit on Monday if there are no further comments.

          Show
          templedf Daniel Templeton added a comment - +1 I'll commit on Monday if there are no further comments.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
          +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.
          +1 mvninstall 6m 46s trunk passed
          +1 compile 0m 31s trunk passed
          +1 checkstyle 0m 20s trunk passed
          +1 mvnsite 0m 38s trunk passed
          +1 mvneclipse 0m 17s trunk passed
          +1 findbugs 0m 57s trunk passed
          +1 javadoc 0m 22s trunk passed
          +1 mvninstall 0m 30s the patch passed
          +1 compile 0m 29s the patch passed
          +1 javac 0m 29s the patch passed
          +1 checkstyle 0m 17s the patch passed
          +1 mvnsite 0m 34s the patch passed
          +1 mvneclipse 0m 14s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 0s the patch passed
          +1 javadoc 0m 18s the patch passed
          -1 unit 38m 28s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 15s The patch does not generate ASF License warnings.
          53m 26s



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



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue YARN-4907
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835844/YARN-4907.002.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux a62381955214 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 1b79c41
          Default Java 1.8.0_101
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-YARN-Build/13639/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-YARN-Build/13639/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13639/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated. +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. +1 mvninstall 6m 46s trunk passed +1 compile 0m 31s trunk passed +1 checkstyle 0m 20s trunk passed +1 mvnsite 0m 38s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 0m 57s trunk passed +1 javadoc 0m 22s trunk passed +1 mvninstall 0m 30s the patch passed +1 compile 0m 29s the patch passed +1 javac 0m 29s the patch passed +1 checkstyle 0m 17s the patch passed +1 mvnsite 0m 34s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 0s the patch passed +1 javadoc 0m 18s the patch passed -1 unit 38m 28s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 53m 26s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.applicationsmanager.TestAMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue YARN-4907 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835844/YARN-4907.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a62381955214 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 1b79c41 Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-YARN-Build/13639/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-YARN-Build/13639/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13639/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          yufeigu Yufei Gu added a comment -

          The failed test is unrelated.

          Show
          yufeigu Yufei Gu added a comment - The failed test is unrelated.
          Hide
          templedf Daniel Templeton added a comment -

          Thank you for the patch, Yufei Gu, and the review, Miklos Szegedi. Committed to branch-2 and trunk.

          Show
          templedf Daniel Templeton added a comment - Thank you for the patch, Yufei Gu , and the review, Miklos Szegedi . Committed to branch-2 and trunk.
          Hide
          yufeigu Yufei Gu added a comment -

          Thank you for the review and commit, Daniel Templeton. Thanks for the review Miklos Szegedi.

          Show
          yufeigu Yufei Gu added a comment - Thank you for the review and commit, Daniel Templeton . Thanks for the review Miklos Szegedi .
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10733 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10733/)
          YARN-4907. Make all MockRM#waitForState consistent. (Contributed by (templedf: rev cc2c993a8af6265b9881550501fd16f783519e03)

          • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockRM.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10733 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10733/ ) YARN-4907 . Make all MockRM#waitForState consistent. (Contributed by (templedf: rev cc2c993a8af6265b9881550501fd16f783519e03) (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockRM.java

            People

            • Assignee:
              yufeigu Yufei Gu
              Reporter:
              yufeigu Yufei Gu
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development