One way to handle this would be to (1) sleep for a smaller duration and (2) check for states matching the state provided or later ones. The other option is to either notify waiters when an app reaches a state, or capture all the states an app visits.

Attaching a first patch.

1. Making polling interval smaller(100msec).
2. Adding waitStateExecutor for polling the state.
3. Using CountDownLatch.

Are 2 and 3 required here?

If we use the CountDownLatch, we can understand the timeout value more easily:

latch.await(40, TimeUnit.SECONDS))

If I shouldn't do this here, I'll revert it.

About the MockAM#waitForState, should we fix it on this JIRA?

Using the latch would make sense if the threads have any other advantage; otherwise, I would leave both out and control time waited through number of iterations of the for loop. Fixing MockAM#waitFor here would be good too.

We should make sure the cumulative wait time stays at least as long as what it is now to avoid any test failures.

Other than the smaller sleep, we should also handle the case where the App or AppAttempt enters the required state and then moves to a latter state. e.g. App moving to RUNNING state when we are waiting for it to get ACCEPTED.

Thanks for your suggestion, Karthik. I agree with that we should use wait-by-loop approach in this case. I'll update it in a next patch.

Other than the smaller sleep, we should also handle the case where the App or AppAttempt enters the required state and then moves to a latter state. e.g. App moving to RUNNING state when we are waiting for it to get ACCEPTED.

I spent some time how I can implement it - how about using variadic as a argument of waitFor()? This will work without changing code.

  public void waitForState(RMAppAttemptState... finalStatesArray)
      throws Exception {
    EnumSet<RMAppAttemptState> finalStates
        = EnumSet.copyOf(Arrays.asList(finalStatesArray));
    while (!finalStates.contains(attempt.getAppAttemptState()) && !isTimeout() {
    }

Updating a patch based on a discussion with Karthik.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12685534/YARN-2921.002.patch
against trunk revision e227fb8.

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

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

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

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

-1 core tests. The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:

org.apache.hadoop.yarn.server.resourcemanager.security.TestClientToAMTokens
org.apache.hadoop.yarn.server.resourcemanager.webapp.TestRMWebServices
org.apache.hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesNodeLabels
org.apache.hadoop.yarn.server.resourcemanager.security.TestAMRMTokens
org.apache.hadoop.yarn.server.resourcemanager.applicationsmanager.TestAMRestart
org.apache.hadoop.yarn.server.resourcemanager.TestApplicationCleanup
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.TestQueueMappings
org.apache.hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesCapacitySched
org.apache.hadoop.yarn.server.resourcemanager.TestRMHA
org.apache.hadoop.yarn.server.resourcemanager.TestKillApplicationWithRMHA
org.apache.hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesDelegationTokens
org.apache.hadoop.yarn.server.resourcemanager.TestApplicationMasterLauncher
org.apache.hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesAppsModification
org.apache.hadoop.yarn.server.resourcemanager.TestContainerResourceUsage
org.apache.hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer
org.apache.hadoop.yarn.server.resourcemanager.TestMoveApplication
org.apache.hadoop.yarn.server.resourcemanager.TestSubmitApplicationWithRMHA
org.apache.hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesNodes

The following test timeouts occurred in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:

org.apache.hadoop.yarn.server.resourcemanager.TestRMRestart
org.apache.hadoop.yarn.server.resourcemanager.TestWorkPreservingRMRestart
org.apache.hadoop.yarn.server.resourcemanager.TestRM
org.apache.hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesApps
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.TestCapacitySchedulerQueueACLs
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.TestCapacityScheduler
org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.TestFairSchedulerQueueACLs
org.apache.hadoop.yarn.server.resourcemanager.TestApplicationMasterService
org.apache.hadoop.yarn.server.resourcemanager.TestFifoScheduler

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

Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6021//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6021//console

This message is automatically generated.

Oh, I found that the duration of sleeping is wrong. Fixing sleep time.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12685553/YARN-2921.003.patch
against trunk revision e227fb8.

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

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

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

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

-1 core tests. The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:

org.apache.hadoop.yarn.server.resourcemanager.TestRMRestart
org.apache.hadoop.yarn.server.resourcemanager.applicationsmanager.TestAMRestart
org.apache.hadoop.yarn.server.resourcemanager.TestContainerResourceUsage
org.apache.hadoop.yarn.server.resourcemanager.TestWorkPreservingRMRestart
org.apache.hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesApps

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

Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6023//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6023//console

This message is automatically generated.

Make timeouts smaller and fix tests failure.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12696827/YARN-2921.004.patch
against trunk revision c4980a2.

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

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

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

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

-1 core tests. The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:

org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.TestAllocationFileLoaderService

Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6523//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6523//console

This message is automatically generated.

The test failure looks not related to my patch. Karthik Kambatla, could you take a look? This patch deals with "tests can get too slow" problem by shortening polling period. This value looks minimum value of sleep to success the tests, though I haven't inspected the reasons yet.

e.g. App moving to RUNNING state when we are waiting for it to get ACCEPTED.

I also found that it is a bit challenging to do this automatically - maybe Graph class is useful to know all transition, but I haven't done this not yet.

Rekicked Jenkins.

-1 overall

This message was automatically generated.

Patch LGTM generally, is failure related to your patch? Tsuyoshi Ozawa.

Wangda Tan thank you for pinging me. Yes, it looks related. Let me survey....

-1 overall

This message was automatically generated.

Fixing the test case.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Wangda Tan, could you take a look latest patch? This patch includes following changes:

1. I found that Thread.isAlive() doesn't work correctly in AsyncDispatcher when the state of thread is not referenced in busy loop. This problem looks a timing bug, and the change of MockRM reproduced the problem. It would be bug of JIT complication or something. but I don't track it deeply currently.

         while (!drained && eventHandlingThread.isAlive()) {
           waitForDrained.wait(1000);
-          LOG.info("Waiting for AsyncDispatcher to drain.");
+          LOG.info("Waiting for AsyncDispatcher to drain. Thread state is :" +
+              eventHandlingThread.getState());
         }

2. The failure of TestAMRestart is because of the change of period of polling. I made the timeout of submitApps larger.

Hi Tsuyoshi Ozawa,
Some comments:

• In MockAM.waitForState, I'm not very understand about the change: 1. why minWaitMSec is needed? 2. Why fail the method if if (waitedMsecs >= timeoutMsecs) is true? I think it should check now-state against expected state.
• In two MockRM.waitForState method, I think we should also check app.getState() instead of time, correct?
• In TestRMRestart, you can use GenericTestUtils.waitFor instead.

Wangda Tan thank you for the comments.

1. why minWaitMSec is needed?

If minWaitMsec is removed, some test fails currently. It looks that some tests depend on sleep time in MockRM/MockAM. We can fix it on another JIRA.

2. Why fail the method if if (waitedMsecs >= timeoutMsecs) is true? I think it should check now-state against expected state.

• In two MockRM.waitForState method, I think we should also check app.getState() instead of time, correct?

Let me explain. The state check is done in the while statement. After that, the additional state transitions can occur. It results false-positive assertions - waitFor() waits for the targeted state, but assertions can fail. To prevent this kind of false positive assertions, I don't add the state check after the while loop. Does it make sense to you?

In TestRMRestart, you can use GenericTestUtils.waitFor instead.

Fixed.

Fixed.

Fixed on v8 patch.

-1 overall

This message was automatically generated.

Hmm, all test failures say "ZipException: invalid block type", and it looks not related to the patch. Kicking Jenkins again after resolving the issue.

Resubmitting a patch again.

-1 overall

This message was automatically generated.

Wangda Tan all tests are green. Could you check v8 patch and my comments?
https://issues.apache.org/jira/browse/YARN-2921?focusedCommentId=14539843&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14539843

Thanks for explanation, Tsuyoshi Ozawa. Latest patch LGTM, checking it in..

Just committed to branch-2/trunk. Thanks Tsuyoshi Ozawa working on this issue and Karthik Kambatla reporting it.

FAILURE: Integrated in Hadoop-trunk-Commit #7818 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7818/)
YARN-2921. Fix MockRM/MockAM#waitForState sleep too long. (Tsuyoshi Ozawa via wangda) (wangda: rev 341a476812015d0d584b198b451ea9458645a47c)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestApplicationMasterService.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockAM.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestRMRestart.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/applicationsmanager/TestAMRestart.java
• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/event/AsyncDispatcher.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockRM.java

Thanks Tsuyoshi for fixing this. Just curious - do we know how much improvement this leads to when running RM tests.

Yeah, TestRMRestart speeded up about 30 seconds.

Before: 393 secs
After: 367 secs

There is a possibility to improve the test time by making sleep time smaller and removing minWaitMSec.

Thanks for your review and comments, Wangda and Karthik!

FAILURE: Integrated in Hadoop-Yarn-trunk #927 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/927/)
YARN-2921. Fix MockRM/MockAM#waitForState sleep too long. (Tsuyoshi Ozawa via wangda) (wangda: rev 341a476812015d0d584b198b451ea9458645a47c)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/applicationsmanager/TestAMRestart.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockAM.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockRM.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/event/AsyncDispatcher.java
• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestApplicationMasterService.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestRMRestart.java

SUCCESS: Integrated in Hadoop-Yarn-trunk-Java8 #196 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/196/)
YARN-2921. Fix MockRM/MockAM#waitForState sleep too long. (Tsuyoshi Ozawa via wangda) (wangda: rev 341a476812015d0d584b198b451ea9458645a47c)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/event/AsyncDispatcher.java
• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/applicationsmanager/TestAMRestart.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockAM.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestRMRestart.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestApplicationMasterService.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockRM.java

FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #195 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/195/)
YARN-2921. Fix MockRM/MockAM#waitForState sleep too long. (Tsuyoshi Ozawa via wangda) (wangda: rev 341a476812015d0d584b198b451ea9458645a47c)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockAM.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockRM.java
• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestRMRestart.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestApplicationMasterService.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/event/AsyncDispatcher.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/applicationsmanager/TestAMRestart.java

FAILURE: Integrated in Hadoop-Hdfs-trunk #2125 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2125/)
YARN-2921. Fix MockRM/MockAM#waitForState sleep too long. (Tsuyoshi Ozawa via wangda) (wangda: rev 341a476812015d0d584b198b451ea9458645a47c)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/event/AsyncDispatcher.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/applicationsmanager/TestAMRestart.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockAM.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestApplicationMasterService.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestRMRestart.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockRM.java
• hadoop-yarn-project/CHANGES.txt

FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #185 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/185/)
YARN-2921. Fix MockRM/MockAM#waitForState sleep too long. (Tsuyoshi Ozawa via wangda) (wangda: rev 341a476812015d0d584b198b451ea9458645a47c)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestApplicationMasterService.java
• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockAM.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockRM.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/applicationsmanager/TestAMRestart.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/event/AsyncDispatcher.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestRMRestart.java

SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2143 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2143/)
YARN-2921. Fix MockRM/MockAM#waitForState sleep too long. (Tsuyoshi Ozawa via wangda) (wangda: rev 341a476812015d0d584b198b451ea9458645a47c)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestRMRestart.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/applicationsmanager/TestAMRestart.java
• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockRM.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockAM.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/event/AsyncDispatcher.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestApplicationMasterService.java