[
https://issues.apache.org/jira/browse/YARN-2009?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15613046#comment-15613046
]
Carlo Curino commented on YARN-2009:
------------------------------------
There is lots going on, so I am partially trusting [~leftnoteasy] and
[~eepayne] long reviewing process.
I think I can mostly get what you are trying to do, and the patch mostly looks
good.
Please fix the checkstyles, as well as consider the following:
# In
{{IntraQueuePreemptionComputePlugin.getResourceDemandFromAppsPerQueue(LeafQueue
leafQueue, String partition)}} ask to pass in a {{LeafQueue}}, but the only
implementor {{FifoIntraQueuePreemptionPlugin}} seems only to care about the
queue name. Can we simply pass in a String? I am concerned we bleed references
to queues and apps a bit much.
# Readability: you call {{queueTotalUnassigned}} a quantity that is currently
assigned, but you are considering "reassignable"... the naming is misleading I
think. This happens in {{FifoIntraQueuePreemptionPlugin}} and its callers.
# more flow-level comments / explanations can help understand the class
relationships and the intents of various methods/steps.
# comments and tests are not always consistent in
{{TestProportionalCapacityPreemptionPolicyIntraQueue}}... e.g., the first split
of guaranteed resources is different in comment and string used to config test.
Also please add more comments (e.g., {{testPreemptionWithTwoUsers}} what is try
to achieve?).
# question: in
{{TestProportionalCapacityPreemptionPolicyIntraQueue.testSimpleIntraQueuePreemption()}}
why is app4 seeing
# generally for the {{TestProportionalCapacityPreemptionPolicyIntraQueue}} it
is nice to have spelled-out cases like you have, but it also makes sense for
coverage to have a large that does many large randomly generated tests, and for
which you check simple invariants (e.g., no preemption within priority, or
priority invertions etc)... This might help unveil corner cases before we hit
prod deployments.
# is {{maxIntraQueuePreemptableLimit}} set to 50% a reasonable default? it
seems maybe high?
> Intra-queue preemption for app priority support
> ProportionalCapacityPreemptionPolicy
> ------------------------------------------------------------------------------------
>
> Key: YARN-2009
> URL: https://issues.apache.org/jira/browse/YARN-2009
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: capacityscheduler
> Reporter: Devaraj K
> Assignee: Sunil G
> Labels: oct16-medium
> Attachments: YARN-2009.0001.patch, YARN-2009.0002.patch,
> YARN-2009.0003.patch, YARN-2009.0004.patch, YARN-2009.0005.patch,
> YARN-2009.0006.patch, YARN-2009.0007.patch, YARN-2009.0008.patch,
> YARN-2009.0009.patch, YARN-2009.0010.patch, YARN-2009.0011.patch,
> YARN-2009.0012.patch, YARN-2009.0013.patch, YARN-2009.0014.patch,
> YARN-2009.0015.patch
>
>
> While preempting containers based on the queue ideal assignment, we may need
> to consider preempting the low priority application containers first.
--
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
