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

Jian He commented on YARN-3983:
-------------------------------

- I suggest  not set the state implicitly in the constructor. It’s quite 
confusing which constructor indicates which state. Setting it explicitly by 
caller makes code easier to read.
{code}
  public ContainerAllocation(RMContainer containerToBeUnreserved) {
    this(containerToBeUnreserved, null, AllocationState.QUEUE_SKIPPED);
  }
  public ContainerAllocation(RMContainer containerToBeUnreserved,
      Resource resourceToBeAllocated) {
    this(containerToBeUnreserved, resourceToBeAllocated,
        AllocationState.SUCCEEDED);
  }
{code}
- reserved container returns SUCCEEDED state?
{code}
ContainerAllocation result =
    new ContainerAllocation(null, 
request.getCapability());
result.reserved = true;
result.containerNodeType = 
type;
{code}
Earlier below code will not be invoked for reserved container, now it gets 
invoked 
{code}
 if (allocationResult.state == AllocationState.SUCCEEDED) {
      // Don't reset scheduling opportunities for offswitch assignments
      // otherwise the app will be delayed for each non-local assignment.
      // This helps apps with many off-cluster requests schedule faster.
      if (allocationResult.containerNodeType != NodeType.OFF_SWITCH) {
        if (LOG.isDebugEnabled()) {
          LOG.debug("Resetting scheduling opportunities");
        }
        application.resetSchedulingOpportunities(priority);
      }
      
      // Non-exclusive scheduling opportunity is different: we need reset
      // it every time to make sure non-labeled resource request will be
      // most likely allocated on non-labeled nodes first.
      
application.resetMissedNonPartitionedRequestSchedulingOpportunity(priority);
    }
{code}
- AllocationState#SUCCEEDED -> ALLOCTED.
- reserved boolean flag can be changed to be Allocateion#RESERVED state.
- CSAssignment#NULL_ASSIGNMENT not used, remove 
- comments does not match method name
{code} * doAllocation needs to handle following stuffs: {code} .
- Below code was originally outside of the priorities loop
{code}
if (SchedulerAppUtils.isBlacklisted(application, node, LOG)) {
  return 
ContainerAllocation.APP_SKIPPED;
}
{code}
-  below code can be changed to use null check ?
{code}
if (Resources.greaterThan(rc, clusterResource,
    
assigned.getResourceToBeAllocated(), Resources.none())) {

{code}
- move the for loop into {{applicationContainerAllocator.allocate}} method
{code}
for (Priority priority : getPriorities()) {
{code}

- return ContainerAllocation.QUEUE_SKIPPED, though logically correct, 
semantically is incorrect, it should return Priority_Skipped
{code}
  private ContainerAllocation assignNodeLocalContainers(
      Resource clusterResource, ResourceRequest nodeLocalResourceRequest,
      FiCaSchedulerNode node, Priority priority, RMContainer reservedContainer,
      SchedulingMode schedulingMode, ResourceLimits currentResoureLimits) {
    if (canAssign(priority, node, NodeType.NODE_LOCAL, reservedContainer)) {
      return assignContainer(clusterResource, node, priority,
          nodeLocalResourceRequest, NodeType.NODE_LOCAL, reservedContainer,
          schedulingMode, currentResoureLimits);
    }
    return ContainerAllocation.QUEUE_SKIPPED;
  }

      // check if the resource request can access the label
    if (!SchedulerUtils.checkResourceRequestMatchingNodePartition(request,
        node.getPartition(), schedulingMode)) {
      // this is a reserved container, but we cannot allocate it now according
      // to label not match. This can be caused by node label changed
      // We should un-reserve this container.
      return new ContainerAllocation(rmContainer);
    }
{code}
- APP_SKIPPED? original code seems skipping the priority 
{code}
    // Does the application need this resource?
    if (allocatedContainer == null) {
      // Skip this app if we failed to allocate.
      ContainerAllocation ret =
          new ContainerAllocation(allocationResult.containerToBeUnreserved);
      ret.state = AllocationState.APP_SKIPPED;
      return ret;
    }
{code}



> Make CapacityScheduler to easier extend application allocation logic
> --------------------------------------------------------------------
>
>                 Key: YARN-3983
>                 URL: https://issues.apache.org/jira/browse/YARN-3983
>             Project: Hadoop YARN
>          Issue Type: Bug
>            Reporter: Wangda Tan
>            Assignee: Wangda Tan
>         Attachments: YARN-3983.1.patch, YARN-3983.2.patch
>
>
> While working on YARN-1651 (resource allocation for increasing container), I 
> found it is very hard to extend existing CapacityScheduler resource 
> allocation logic to support different types of resource allocation.
> For example, there's a lot of differences between increasing a container and 
> allocating a container:
> - Increasing a container doesn't need to check locality delay.
> - Increasing a container doesn't need to build/modify a resource request tree 
> (ANY->RACK/HOST).
> - Increasing a container doesn't need to check allocation/reservation 
> starvation (see {{shouldAllocOrReserveNewContainer}}).
> - After increasing a container is approved by scheduler, it need to update an 
> existing container token instead of creating new container.
> And there're lots of similarities when allocating different types of 
> resources.
> - User-limit/queue-limit will be enforced for both of them.
> - Both of them needs resource reservation logic. (Maybe continuous 
> reservation looking is needed for both of them).
> The purpose of this JIRA is to make easier extending CapacityScheduler 
> resource allocation logic to support different types of resource allocation, 
> make common code reusable, and also better code organization.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to