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