[ 
https://issues.apache.org/jira/browse/YARN-9041?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16704014#comment-16704014
 ] 

Yufei Gu commented on YARN-9041:
--------------------------------

Hi [~jiwq], the patch v5 looks good in terms of logic. Some nits:
# Can you rename two tests or write comments to clarify their intentions? I 
suppose the goals of two methods are: One can find less AM containers solution 
in the relax locations and the other can't.
# It is a good practice to put the callee methods under the caller methods.
# Can you refactor to create a new method like this? Please remember to 
reorganize the method java doc.  And we probably don't need 
the comment "// Don't preempt AM containers just to satisfy local requests if 
relax // locality is enabled." in that case.
{code}
  /**
   * Iterate through matching
   *  nodes and identify containers to preempt all on one node, also
   *    * optimizing for least number of AM container preemptions. Only nodes
   *    * that match the locality level specified in the {@link ResourceRequest}
   *    * are considered. However, if this would lead to AM preemption, and 
locality
   *    * relaxation is allowed, then the search space is expanded to the 
remaining
   *    * nodes.
   *
   * @param rr
   * @param potentialNodes
   * @return
   */
  private PreemptableContainers getBestPreemptableContainers(ResourceRequest 
rr, List<FSSchedulerNode> potentialNodes) {
    PreemptableContainers bestContainers =
            identifyContainersToPreemptForOneContainer(potentialNodes, rr);


    if (rr.getRelaxLocality()
        && !ResourceRequest.isAnyLocation(rr.getResourceName())
        && bestContainers != null
        && bestContainers.numAMContainers > 0) {
      List<FSSchedulerNode> remainingNodes = 
scheduler.getNodeTracker().getAllNodes();
      remainingNodes.removeAll(potentialNodes);
      PreemptableContainers spareContainers = 
identifyContainersToPreemptForOneContainer(remainingNodes, rr);
      if (spareContainers != null && spareContainers.numAMContainers < 
bestContainers.numAMContainers) {
        bestContainers = spareContainers;
      }
    }
    return bestContainers;
  }
{code}

> Optimize FSPreemptionThread#identifyContainersToPreempt method
> --------------------------------------------------------------
>
>                 Key: YARN-9041
>                 URL: https://issues.apache.org/jira/browse/YARN-9041
>             Project: Hadoop YARN
>          Issue Type: Improvement
>          Components: scheduler preemption
>            Reporter: Wanqiang Ji
>            Assignee: Wanqiang Ji
>            Priority: Major
>         Attachments: YARN-9041.001.patch, YARN-9041.002.patch, 
> YARN-9041.003.patch, YARN-9041.004.patch, YARN-9041.005.patch
>
>
> In FSPreemptionThread#identifyContainersToPreempt method, I suggest if AM 
> preemption, and locality relaxation is allowed, then the search space is 
> expanded to all nodes changed to the remaining nodes. The remaining nodes are 
> equal to all nodes minus the potential nodes.
> Judging condition changed to:
>  # rr.getRelaxLocality()
>  # !ResourceRequest.isAnyLocation(rr.getResourceName())
>  # bestContainers != null
>  # bestContainers.numAMContainers > 0
> If I understand the deviation, please criticize me. thx~



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to