[jira] [Commented] (YARN-5047) Refactor nodeUpdate across schedulers

[Daniel Templeton (JIRA)]

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

Daniel Templeton commented on YARN-5047:
----------------------------------------

Thanks for the patch, [~rchiang]!  A few comments:

* The {{AbstractYarnScheduler.nodeUpdate()}} method is pretty rambly.  Think 
you could refactor it into a couple of more bite-sized methods?
* In this code:

{code}
        Resource rs = container.getAllocatedResource();
        if (rs != null) {
          Resources.addTo(releasedResources, rs);
        }
        rs = container.getReservedResource();
        if (rs != null) {
          Resources.addTo(releasedResources, rs);
        }
{code}

I'd rather not reuse the {{rs}} variable.
* For this comment:

{code}
    // TODO: Fix possible race-condition when request comes in before
    // update is propagated
{code}

is there a JIRA filed?  If so, can you include it in the comment?  (I realize 
that came from the CS code, but it would be nice to clean it up.)
* In this code:

{code}
    if (rmContext.isWorkPreservingRecoveryEnabled()
        && !rmContext.isSchedulerReadyForAllocatingContainers()) {
      return;
    }
    if (Resources.greaterThanOrEqual(resourceCalculator, getClusterResource(),
        node.getUnallocatedResource(), minimumAllocation)) {
...
    }
{code}

I'd much rather you add the first if's conditions to the second if, rather than 
use the if-return.

> Refactor nodeUpdate across schedulers
> -------------------------------------
>
>                 Key: YARN-5047
>                 URL: https://issues.apache.org/jira/browse/YARN-5047
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: capacityscheduler, fairscheduler, scheduler
>    Affects Versions: 3.0.0-alpha1
>            Reporter: Ray Chiang
>            Assignee: Ray Chiang
>         Attachments: YARN-5047.001.patch, YARN-5047.002.patch, 
> YARN-5047.003.patch, YARN-5047.004.patch, YARN-5047.005.patch, 
> YARN-5047.006.patch, YARN-5047.007.patch, YARN-5047.008.patch, 
> YARN-5047.009.patch, YARN-5047.010.patch
>
>
> FairScheduler#nodeUpdate() and CapacityScheduler#nodeUpdate() have a lot of 
> commonality in their code.  See about refactoring the common parts into 
> AbstractYARNScheduler.



--
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