[
https://issues.apache.org/jira/browse/YARN-5830?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15816420#comment-15816420
]
Karthik Kambatla commented on YARN-5830:
----------------------------------------
Comments on the latest patch:
# The patch does not apply cleanly anymore. Can we rebase it please?
# FSPreemptionThread#run: Based on the contract of identifyContainersToPreempt,
it either returns the containers to preempt to return or null. Do we need to
check the size? Can we avoid this?
# moveNonAMContainerFirst: I see the method swaps AM containers with non-AM
ones. However, it is rather long. Aren't we better off having
SchedulerNode#getCopiedListOfRunningContainers to return a list with AM
containers at the end. Based on the current uses of this method, this should
not be a problem. Alternatively, I am fine with implementing another method
that returns the list with AM containers at the end.
# PreemptableContainers:
## The class should be static.
## Rename the field to numAMContainers?
## The constructor has an empty space between the name and parentheses.
## I would rather add a method addContainer that maintains the list and
increments numAMContainers if it is an AM container.
## Maybe, add another field maxAMContainers. That way, addContainer could
return false if it exceeds the maxAMContainers, and you could return null
without additional checks.
# identifyContainersToPreemptOnNode
## The javadoc should provide more details on the logic.
## I would start the args list with FSSchedulerNode and rename last arg to
maxAMContainers. Also, update the javadoc to say the same instead of "smallest
number"
## If we don't get rid of moveNonAMContainerFirst, that should come immediately
after containersToCheck.removeAll without any new lines to group similar code
together.
## When {{request <= potential}}, the containers in preemptableContainers is
the best so far. This is not the final value the caller method returns. So, we
should not call node.addContainersForPreemption here, but instead in the
caller.
# identifyContainersToPreempt: Can the code be simplified as follows:
{code}
...
Containers bestContainers = null;
int maxAMContainers = Integer.MAX_VALUE;
for (
...
identifyPreemptableContainers(blah, blah, maxAMContainers);
if (preemptableContainers != null) {
if (preemptableContainers.numAMContainers == 0) {
return preemptableContainers.containers;
} else {
bestContainers = preemptableContainers;
maxAMContainers = bestContainers.numAMContainers;
}
}
return bestContainers;
{code}
> Avoid preempting AM containers
> ------------------------------
>
> Key: YARN-5830
> URL: https://issues.apache.org/jira/browse/YARN-5830
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: fairscheduler
> Reporter: Karthik Kambatla
> Assignee: Yufei Gu
> Attachments: YARN-5830.001.patch, YARN-5830.002.patch,
> YARN-5830.003.patch
>
>
> While considering containers for preemption, avoid AM containers unless
> absolutely necessary.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
