[
https://issues.apache.org/jira/browse/YARN-6216?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15881061#comment-15881061
]
Wangda Tan commented on YARN-6216:
----------------------------------
Hi [~asuresh], the patch looks GREAT and simplified a lot of logics which is
added for container resizing, thanks for working on this!
*Major:*
1) It looks like all container change request needs to create a new
containerId, correct? It's not a critical issue, but could potentially make the
container-id becomes very sparse. I think this should be optimizable.
2) Could you elaborate new changes added to
AbstractYarnScheduler#completedContainer?
3) Can we simplify decrease/demote container process: now it is create a new
RMContainer and make changes to existing container when pulled by app, could we
change it in-place which can benefit: A. no need to create a new RMContainer,
B. no need to add them to "outstanding decrease/demote requests", C. pull new
demoted/decreased containers logics can be simplified in
SchedulerApplicationAttempt.
4) Not critical but worth to think: for increase/promote container, it increase
#live-containers by 1 in scheduler and soon decrease it. Since #live-containers
is visible to client, if changes to fix the issue is not too complex, it's
better to optimize this.
*Minor:*
5) SchedulerApplicationAttempt:
- IIUC, createResourceToShed is to assigned to the new RMContainer placeholder
to make scheduler can correctly release resources. Could you add some comments
and rename it to "getResourceOfRMContainerPlaceholder"
- A couple of rename suggestions to {{swapContainer}}: {{updatedContainer}} to
{{tempContainer}}; {{finalResource}} to {{updatedResource}}; {{newContainer}}
to {{updatedContainer}}; {{rmContainer}} to {{tempRmContainer}}
- Typo: updatType -> updateType
6) ContainerAllocator:
Not necessary to call regularContainerAllocator.assignContainers twice, once
should be enough. And not necessary to check reservedContainer anymore.
7) Not caused by your patch, testOrderOfIncreaseContainerRequestAllocation and
testIncreaseContainerRequestGetPreferrence are completely identical, you can
remove one of them.
8) Several unused methods in AppSchedulingInfo:
- hasRequestLabelChanged/getFirstSchedulingPlacementSet
> Unify Container Resizing code paths with Container Updates making it
> scheduler agnostic
> ---------------------------------------------------------------------------------------
>
> Key: YARN-6216
> URL: https://issues.apache.org/jira/browse/YARN-6216
> Project: Hadoop YARN
> Issue Type: Bug
> Components: capacity scheduler, fairscheduler, resourcemanager
> Affects Versions: 3.0.0-alpha2
> Reporter: Arun Suresh
> Assignee: Arun Suresh
> Fix For: 3.0.0-alpha3
>
> Attachments: YARN-6216.001.patch, YARN-6216.002.patch
>
>
> YARN-5959 introduced an {{ContainerUpdateContext}} which can be used to
> update the ExecutionType of a container in a scheduler agnostic manner. As
> mentioned in that JIRA, extending that to encompass Container resizing is
> trivial.
> This JIRA proposes to remove all the CapacityScheduler specific code paths.
> (CapacityScheduler, CSQueue, FicaSchedulerApp etc.) and modify the code to
> use the framework introduced in YARN-5959 without any loss of functionality.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
