[
https://issues.apache.org/jira/browse/YARN-2056?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14208990#comment-14208990
]
Wangda Tan commented on YARN-2056:
----------------------------------
Minor comments:
1) Typo of comment in cloneQueue:
bq. + // I have no untouchable over-capicity (that is, all of my over-
capici.. -> capacity
2)
{code}
+ // If my children's preemptable over-capacity is >= my over-capacity,
+ // I have no untouchable over-capicity (that is, all of my over-
+ // capacity resources are preemptable). Otherwise, to calculate my
+ // untouchable over-capacity, subtract my children's preemptable over-
+ // capacity from my over-capacity.
{code}
The logic should be correct, but I think it might be simpler to say:
untouchableExtra = max(extra - childrenPreemptable, 0) and as same as the code.
:)
3)
bq. + public double getIdealPctOfGuaranteed(TempQueue q)
The method doesn't need to be public anymore
4)
Does it possible there's only one queue in getMostUnderservedQueues? If so, you
should check if q2 is null
Included review for tests in this turn:
1) testDisablePreemptionOverCapPlusPending
Should disable queueB instead of queueA? Currently, the test will preempt from
appB not matter if preemption disabled for queueA or not.
2) changes for testHierarchicalLarge:
{code}
- verify(mDisp, times(9)).handle(argThat(new IsPreemptionRequestFor(appA)));
- verify(mDisp, times(4)).handle(argThat(new IsPreemptionRequestFor(appE)));
+ verify(mDisp, times(6)).handle(argThat(new IsPreemptionRequestFor(appA)));
+ verify(mDisp, times(6)).handle(argThat(new IsPreemptionRequestFor(appE)));
{code}
I'm a little concern about this change, even if we considering round error,
appA should be taken about 9-10 resources, 9->6 seems some potential bug caused
issue, could you double check if it works as expected? (Without affect the
normal preemption logic).
3) As above
{code}
- // Since AM containers of appC and appD are saved, 2 containers from appB
+ // Since AM containers of appC and appD are saved, 1 containers from appB
// has to be preempted
{code}
This is also a dangerous change, since it breaks the original expectation about
skip AM container test.
I suggest you can take some investigation about why some original numbers need
to be changed, if it is just a round problem, that should be fine, but we
should avoid behavior changes.
> Disable preemption at Queue level
> ---------------------------------
>
> Key: YARN-2056
> URL: https://issues.apache.org/jira/browse/YARN-2056
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: resourcemanager
> Affects Versions: 2.4.0
> Reporter: Mayank Bansal
> Assignee: Eric Payne
> Attachments: YARN-2056.201408202039.txt, YARN-2056.201408260128.txt,
> YARN-2056.201408310117.txt, YARN-2056.201409022208.txt,
> YARN-2056.201409181916.txt, YARN-2056.201409210049.txt,
> YARN-2056.201409232329.txt, YARN-2056.201409242210.txt,
> YARN-2056.201410132225.txt, YARN-2056.201410141330.txt,
> YARN-2056.201410232244.txt, YARN-2056.201410311746.txt,
> YARN-2056.201411041635.txt, YARN-2056.201411072153.txt,
> YARN-2056.201411122305.txt
>
>
> We need to be able to disable preemption at individual queue level
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
