[ https://issues.apache.org/jira/browse/YARN-4807?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15221876#comment-15221876 ]
Karthik Kambatla commented on YARN-4807: ---------------------------------------- Few comments, mostly nits: MockRM # Can we use "private static final" instead of static final private" for the new constants? # waitForState(ApplicationId appId, RMAppState finalState): ## move {{int loop = 0}} to the for loop ## After the loop, just add an assert that verifies the "state" irrespective of why we exited the loop. # waitForState(RMAppAttempt attempt, RMAppAttemptState finalState, int timeoutMsecs) ## move {{int loop = 0}} to the for loop ## After the for loop, why are we sleeping for the remaining time? I would think this slows down tests significantly. ## just add an assert that verifies the "state" irrespective of why we exited the loop. # waitForContainerToComplete - do we not have any timeout on this? I recommend adding one. # Same goes for watiForNewAMToLaunchAndRegister # waitForState(Collection<MockNM> nms, ContainerId containerId, RMContainerState containerState, int timeoutMsecs) ## move {{int loop = 0}} to the first for loop ## assertNotNull need not be guarded by if (container == null) ## Move the if check for (container == null) to one of the conditions for the following for. > MockAM#waitForState sleep duration is too long > ---------------------------------------------- > > Key: YARN-4807 > URL: https://issues.apache.org/jira/browse/YARN-4807 > Project: Hadoop YARN > Issue Type: Sub-task > Affects Versions: 2.8.0 > Reporter: Karthik Kambatla > Assignee: Yufei Gu > Labels: newbie > Attachments: YARN-4807.001.patch, YARN-4807.002.patch, > YARN-4807.003.patch, YARN-4807.004.patch, YARN-4807.005.patch, > YARN-4807.006.patch > > > MockAM#waitForState sleep duration (500 ms) is too long. Also, there is > significant duplication with MockRM#waitForState. -- This message was sent by Atlassian JIRA (v6.3.4#6332)