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

Eric Payne commented on YARN-4945:
----------------------------------

Thanks [~sunilg]. I have a few review comments for patch v0:

- {{IntraQueuePreemptableResourceCalculator#computeIntraQueuePreemptionDemand}}:
-- Neither of the parameters are used ({{clusterResource}} 
{{totalPreemptedResourceAllowed}})
-- {{queueNames}} can be null, which causes an NPE in {{for (String queueName : 
queueNames)}}
-- {{leafQueue}} will be null if {{tq}} represents a parent queue, which causes 
NPE when dereferenced later.
-- {{CapacitySchedulerConfiguration.USED_CAPACITY_THRESHOLD_FOR_PREEMPTION}}: 
[~leftnoteasy] indicated above that this property is similar to 
{{MAX_IGNORED_OVER_CAPACITY}}, but I'm not sure I understand how that applies 
to intra-queue preemption. The comparison is not between queues at this point, 
it's between apps or users. In patch v0, the following code has the effect of 
only allowing preemption if the queue's used resources are below 
{{USED_CAPACITY_THRESHOLD_FOR_PREEMPTION}}, which defaults to 30%. It doesn't 
make sense to me to limit intra-queue preemption based on how much of the 
queue's guaranteed resources are used.
{code}
        if (leafQueue.getUsedCapacity() < context
            .getUsedCapThresholdForPreemptionPerQueue()) {
          continue;
        }
{code}

- {{IntraQueueCandidatesSelector#selectCandidates}}:
-- If {{queueName}} is not a leaf queue, {{leafQueue}} will be null and cause 
NPE when dereferenced later:
{code}
      // 4. Iterate from most under-served queue in order.
      for (String queueName : queueNames) {
        LeafQueue leafQueue = preemptionContext.getQueueByPartition(queueName,
            RMNodeLabelsManager.NO_LABEL).leafQueue;
{code}
-- Very tiny nit: Remove the word {{get}} from the following:
{code}
    // 3. Loop through all partitions to get calculate demand
{code}

- {{AbstractPreemptableResourceCalculator}}:
-- Since {{TAComparator}} is specifically comparing app priority, can it be 
renamed to something like {{TAPriorityComparator}}?

- {{TempAppPerQueue#toString}}:
-- Small nit: Can the {{toString}} method print ApplicationID and rename 
{{NAME}} to {{QUEUENAME}}?


> [Umbrella] Capacity Scheduler Preemption Within a queue
> -------------------------------------------------------
>
>                 Key: YARN-4945
>                 URL: https://issues.apache.org/jira/browse/YARN-4945
>             Project: Hadoop YARN
>          Issue Type: Bug
>            Reporter: Wangda Tan
>         Attachments: Intra-Queue Preemption Use Cases.pdf, 
> IntraQueuepreemption-CapacityScheduler (Design).pdf, YARN-2009-wip.2.patch, 
> YARN-2009-wip.patch, YARN-2009-wip.v3.patch, YARN-2009.v0.patch
>
>
> This is umbrella ticket to track efforts of preemption within a queue to 
> support features like:
> YARN-2009. YARN-2113. YARN-4781.



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

Reply via email to