[ 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: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org