Wangda Tan commented on YARN-2933:

Hi [~mayank_bansal],
Overall method looks good to me, thanks for update.

Some comments about implementation details:
1) You can use {{clusterResource = 
 clusterResource);}} instead of get {{clusterResource = clusterResource - 
2) {{lm.getNodeLabels();}} will copy the node to labels map, so it will be 
expensive when decide to preempt every container. I suggest we can get a 
node-to-labels map *at the beginning of {{editSchedule}}*, this will presume 
node-to-labels is not changed during the preemption policy execution. But I 
think it will be reasonable since we presume queue-resource is not changed 
dring preemption policy execution as well. In addition, {{isLabeledContainer}} 
can leverage the map instead of loop every entry.

Regarding test,
I think this test covers one case, which is _do no preempt containers from NMs 
with label_. Another case I think need cover is verify ideal_allocation changed 
according to this patch. An example is:

cluster.no_label.resource = 100
cluster.label-x.resource = 100

root.A.capacity = 40
root.A.label-x.capacity = 50
root.A.no_label.used = 40
root.A.label-x.used = 50

root.B.capacity = 40
root.B.label-x.capacity = 50
root.B.no_label.used = 50 
root.B.label-x.used = 0

root.C.capacity = 20
root.C.pending = 10
root.C.used = 10

root.C should preempt 10 from B instead of from A. Even if A's total used 
resource = 90, but A's no-label used resource still because guaranteed no-label 

Does this make sense to you?

> Capacity Scheduler preemption policy should only consider capacity without 
> labels temporarily
> ---------------------------------------------------------------------------------------------
>                 Key: YARN-2933
>                 URL: https://issues.apache.org/jira/browse/YARN-2933
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: capacityscheduler
>            Reporter: Wangda Tan
>            Assignee: Mayank Bansal
>         Attachments: YARN-2933-1.patch, YARN-2933-2.patch, YARN-2933-3.patch
> Currently, we have capacity enforcement on each queue for each label in 
> CapacityScheduler, but we don't have preemption policy to support that. 
> YARN-2498 is targeting to support preemption respect node labels, but we have 
> some gaps in code base, like queues/FiCaScheduler should be able to get 
> usedResource/pendingResource, etc. by label. These items potentially need to 
> refactor CS which we need spend some time carefully think about.
> For now, what immediately we can do is allow calculate ideal_allocation and 
> preempt containers only for resources on nodes without labels, to avoid 
> regression like: A cluster has some nodes with labels and some not, assume 
> queueA isn't satisfied for resource without label, but for now, preemption 
> policy may preempt resource from nodes with labels for queueA, that is not 
> correct.
> Again, it is just a short-term enhancement, YARN-2498 will consider 
> preemption respecting node-labels for Capacity Scheduler which is our final 
> target. 

This message was sent by Atlassian JIRA

Reply via email to