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

Wangda Tan commented on YARN-2933:
----------------------------------

Hi [~mayank_bansal],
Thanks for updating,

In Proportional...Policy, some minor comments
1. {{getNodeLabels}} is nobody using it, should be remove it.

2. {{setNodeLabels}} is too simple to be a method, suggest to remove it too.

3. {{getNonLabeledResources}} should be private

4. {{isLabeledContainer}} could write like 
{code}
  private boolean isLabeledContainer(RMContainer c) {
    return labels.containsKey(c.getAllocatedNode());
  }
{code}
Avoid traversing of all keys,
I suggest to remove this method since it's too simple. At least, it should be 
private.

In Test, currently the {{testIdealAllocationForLabels}} is not correct. In your 
test, queueA/B has total guaranteed *NON_LABELED* resource 100, they used 100 
*NON_LABELED* resource, but {{NodeLabelsManager.getResourceByLabel(no-label)}} 
is only 80. (non-labeled-used/configured-resource > 
NodeLabelsManager.ResourceByLabel(no-label))

One thing need worth to take care is, if we don't do anything on 
TestPro..Policy mocking queues and applications. All used/configured capacities 
are *NON_LABELED* capacity.

I suggest to write test like:

{code}
  @Test
  public void testIdealAllocationForLabels() {
    int[][] qData = new int[][] {
    // / A B
        { 80, 40, 40 }, // abs
        { 80, 80, 80 }, // maxcap
        { 80, 80, 0 }, // used
        { 70, 20, 50 }, // pending
        { 0, 0, 0 }, // reserved
        { 5, 4, 1 }, // apps
        { -1, 1, 1 }, // req granularity
        { 2, 0, 0 }, // subqueues
    };
    setAMContainer = true;
    setLabelContainer = true;
    Map<NodeId, Set<String>> labels = new HashMap<NodeId, Set<String>>();
    NodeId node = NodeId.newInstance("node1", 0);
    Set<String> labelSet = new HashSet<String>();
    labelSet.add("x");
    labels.put(node, labelSet);
    when(lm.getNodeLabels()).thenReturn(labels);
    ProportionalCapacityPreemptionPolicy policy = buildPolicy(qData);
    // Subtracting Label X resources from cluster resources
    when(lm.getResourceByLabel(anyString(), any(Resource.class))).thenReturn(
        Resources.clone(Resource.newInstance(80, 0)));
    clusterResources.setMemory(100);
    policy.editSchedule();

    // By skipping AM Container and Labeled container, all other 18 containers
    // of appD will be
    // preempted
    verify(mDisp, times(18)).handle(argThat(new IsPreemptionRequestFor(appD)));

    // By skipping AM Container and Labeled container, all other 18 containers
    // of appC will be
    // preempted
    verify(mDisp, times(18)).handle(argThat(new IsPreemptionRequestFor(appC)));

    // rest 4 containers from appB will be preempted
    verify(mDisp, times(4)).handle(argThat(new IsPreemptionRequestFor(appB)));
    setAMContainer = false;
    setLabelContainer = false;
  }
{code}
Now, configured *NON_LABELED* resource is 80, before entering 
policy.editSchedule, {{clusterResources.setMemory(100);}}. Which makes 
clusterResource > non-labeled-resource And in computation, it will only 
consider clusterResource is 80 after {{getNonLabeledResources}}.

And could you take a look at findbugs warning.

Thoughts?

> 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, 
> YARN-2933-4.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
(v6.3.4#6332)

Reply via email to