-
Notifications
You must be signed in to change notification settings - Fork 9.2k
YARN-10822. Containers going from New to Scheduled transition for kil… #3632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…led container on recovery
|
🎊 +1 overall
This message was automatically generated. |
| return ContainerState.SCHEDULED; | ||
| } else if (container.recoveredAsKilled && | ||
| container.recoveredStatus == RecoveredContainerStatus.REQUESTED) { | ||
| } else if (container.recoveredAsKilled && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition is getting a little obfuscated.
Probably we should have a static method with this condition and explaining what's the sequence.
| Container c = containers.get(cid); | ||
| assertEquals(ContainerState.DONE, c.getContainerState()); | ||
| app = context.getApplications().get(appId); | ||
| Thread.sleep(1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we wait for something specific instead of sleeping blindly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since ContainerState has been verified with the expected state from StateTransition, sleep is not required.
...org/apache/hadoop/yarn/server/nodemanager/containermanager/TestContainerManagerRecovery.java
Show resolved
Hide resolved
.../test/java/org/apache/hadoop/yarn/server/nodemanager/recovery/NMMemoryStateStoreService.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| } | ||
|
|
||
| static boolean isContainerRecoveredAsKilled(ContainerImpl container) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the { should go in the previous line
...java/org/apache/hadoop/yarn/server/nodemanager/containermanager/container/ContainerImpl.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
…led container on recovery
Description of PR
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?