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
[FLINK-10848][YARN] properly remove YARN ContainerRequest upon container allocation success #7078
Conversation
61d6c05
to
1053b2b
Compare
…r and YarnResourceManager
1053b2b
to
567d69b
Compare
@@ -438,6 +438,8 @@ private void containersAllocated(List<Container> containers) { | |||
numPendingContainerRequests = Math.max(0, numPendingContainerRequests - 1); | |||
LOG.info("Received new container: {} - Remaining pending container requests: {}", | |||
container.getId(), numPendingContainerRequests); | |||
resourceManagerClient.removeContainerRequest(new AMRMClient.ContainerRequest( |
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.
Is this safe to create a new container request object ? Maybe we should call getMatchingRequests
and remove one of the returned results.
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.
getMatchingRequests only filter on priority, resourceName, capability. However, ContainerRequest also contains racks, nodes information.
@@ -361,7 +361,8 @@ public void onContainersAllocated(List<Container> containers) { | |||
"Received new container: {} - Remaining pending container requests: {}", | |||
container.getId(), | |||
numPendingContainerRequests); | |||
|
|||
resourceManagerClient.removeContainerRequest(new AMRMClient.ContainerRequest( |
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.
maybe we can move this removeContainerRequest before this for loop, use a indepedent for loop to do this remove staff.
resourceManagerClient.removeContainerRequest(new AMRMClient.ContainerRequest( | |
resourceManagerClient.removeContainerRequest(new AMRMClient.ContainerRequest( |
Thanks a lot for all the reviews. We've tested the branch in production for some time, merging into the master now. |
…ner allocation success This closes #7078
…ner allocation success This closes #7078
…ner allocation success This closes apache#7078
…ner allocation success This closes apache#7078
What is the purpose of the change
Properly remove YARN ContainerRequest upon container allocation success.
Brief change log
Verifying this change
This change is already covered by existing tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: ( no)Documentation