zhihai xu commented on YARN-3079:

thanks [~leftnoteasy] and [~adhoot]'s review,

addressed the comment from [~adhoot] in the new patch YARN-3079.002.patch.

about the comment from [~leftnoteasy],
bq. 1)  Suggest to change signature of updateMaximumAllocation(SchedulerNode, 
bool) to updateMaximumAllocation(Resource nodeResource, bool), since we only 
uses nodeResource here.
This is discussable. I prefer to keep the current signature because the current 
signature is more flexible and more meaningful for the other parameter(added 
node or removed node). Two nodes can have same nodeResource and you can access 
more information from SchedulerNode.
bq. 2) Change resource for a NM is equivalent to 
{{updateMaximumAllocation(oldNodeResource, false)}} and 
{{updateMaximumAllocation(newNoderesource, true)}}. We can avoid some 
duplicated logic.
I think it is not completely equivalent . because when you call 
{{updateMaximumAllocation(oldNodeResource, false)}}, you supposed the node is 
already removed from the HashMap nodes based on both the implementation of 
updateMaximumAllocation and caller of updateMaximumAllocation. But in the 
context updateNodeResource, the node whose resource to be changed is still in 
the HashMap nodes.
bq. 3) Suggest rename updateMaximumAllocation(void) to 
refreshMaximumAllocation() or other name reflects the behavior: "scan all 
cluster nodes and get maximum allocation".
good suggestion. refreshMaximumAllocation is very good name. addressed this 
comment in the new patch YARN-3079.002.patch.

> Scheduler should also update maximumAllocation when updateNodeResource.
> -----------------------------------------------------------------------
>                 Key: YARN-3079
>                 URL: https://issues.apache.org/jira/browse/YARN-3079
>             Project: Hadoop YARN
>          Issue Type: Bug
>            Reporter: zhihai xu
>            Assignee: zhihai xu
>         Attachments: YARN-3079.000.patch, YARN-3079.001.patch, 
> YARN-3079.002.patch
> Scheduler should also update maximumAllocation when updateNodeResource. 
> Otherwise even the node resource is changed by 
> AdminService#updateNodeResource, maximumAllocation won't be changed.
> Also RMNodeReconnectEvent called from 
> ResourceTrackerService#registerNodeManager will also trigger 
> AbstractYarnScheduler#updateNodeResource being called.

This message was sent by Atlassian JIRA

Reply via email to