[
https://issues.apache.org/jira/browse/YARN-5819?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15651747#comment-15651747
]
Daniel Templeton commented on YARN-5819:
----------------------------------------
bq. Is there a good reason to not use for-loops here?
It doesn't pass my smell test. I wasn't able to turn up any referenceable
condemnation of what you've done. And my suggested alternative doesn't work,
because the expression has to be an {{Iterable}}, not an {{Iterator}}. I don't
have any argument left, other than to appeal to your humanity; think about the
children.
In any case, your {{queue}} variable hides a field from
{{SchedulerApplicationAttempt}}.
In {{FSPreemptionThread.run()}}, the call to
{{containerNode.removeContainerForPreemption(container)}} is pretty important
to keep the {{FSSchedulerNode}} state sane. Do you need to do anything to make
sure it doesn't get skipped because of an exception? In general, I'm a little
nervous about the bookkeeping on the containers to preempt; it seems like
something that could quietly get out of sync and cause issues.
{{FSAppAttempt.preemptedResources}} should be final, as should its neighbors.
You need javadoc for the last three methods in {{FSSchedulerNode}}.
{{FSSchedulerNode.containersForPreemption}} should be final.
In {{FSSchedulerTestBase}}, some javadoc for {{rmNodes}} and {{addNode()}}
would be helpful.
The import of {{org.junit.After}} in {{TestFairSchedulerPreemption}} is grouped
wrong.
{{TestFairSchedulerPreemption.writeAllocFile()}} never throws an
{{IOException}}.
{{TestFairSchedulerPreemption.writeAllocFile()}} is now every bit as ugly as I
feared it would be. What if you pull out the two different conditionals into
separate functions, like:
{code}
private void writeAllocFile() {
...
out.println("<allocations>");
out.println("<queue name=\"preemptable\">");
writePreemptionParams(out);
// Child-1
out.println("<queue name=\"child-1\">");
writeResourceParams(out);
out.println("</queue>");
// Child-2
out.println("<queue name=\"child-2\">");
writeResourceParams(out);
out.println("</queue>");
out.println("</queue>"); // end of preemptable queue
// Queue with preemption disallowed
out.println("<queue name=\"nonpreemptable\">");
out.println("<allowPreemptionFrom>false" +
"</allowPreemptionFrom>");
writePreemptionParams(out);
// Child-1
out.println("<queue name=\"child-1\">");
writeResourceParams(out);
out.println("</queue>");
// Child-2
out.println("<queue name=\"child-2\">");
writeResourceParams(out);
out.println("</queue>");
out.println("</queue>"); // end of nonpreemptable queue
out.println("</allocations>");
...
}
private void writePreemptionParams(PrintWriter out) {
if (fairsharePreemption) {
out.println("<fairSharePreemptionThreshold>1" +
"</fairSharePreemptionThreshold>");
out.println("<fairSharePreemptionTimeout>0" +
"</fairSharePreemptionTimeout>");
} else {
out.println("<minSharePreemptionTimeout>0" +
"</minSharePreemptionTimeout>");
}
}
private void writeResourceParams(PrintWriter out) {
if (!fairsharePreemption) {
out.println("<minResources>4096mb,4vcores</minResources>");
}
}
{code}
Happy medium?
> Verify fairshare and minshare preemption
> ----------------------------------------
>
> Key: YARN-5819
> URL: https://issues.apache.org/jira/browse/YARN-5819
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: fairscheduler
> Affects Versions: 2.9.0
> Reporter: Karthik Kambatla
> Assignee: Karthik Kambatla
> Attachments: yarn-5819.YARN-4752.1.patch, yarn-5819.YARN-4752.2.patch
>
>
> JIRA to track the unit test(s) verifying both fairshare and minshare
> preemption. The tests should verify:
> # preemption within a single leaf queue
> # preemption between sibling leaf queues
> # preemption between non-sibling leaf queues
> # {{allowPreemption = false}} should disallow preemption from a queue
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
