MENG DING commented on YARN-1509:

Hi, [~bikassaha]

Apologize for the late response, I was out traveling and just came back.

bq. A change container request (maybe not supported now) can be increase cpu + 
decrease memory. Hence a built in concept of increase and decrease in the API 
is something I am wary off
>From the design stage of this project, I believe the semantics of "changing 
>container resource" was meant to be either "increase" or "decrease", this was 
>re-enforced with the design choices that successful increase and decrease of 
>resource go through different paths. I have some concerns of extending the 
>semantics to something like "increase cpu + decrease memory" inside one change 
* Decrease resource happens immediately, while increase resource involves 
handing out a token together with a user action to increase on NM. If we extend 
the semantics, we need to educate the user that once a change request is 
approved, it means that the decrease part of the request is effective 
immediately, while the increase part of the request is still pending on user 
action. Could it be too confusing?
* To make matters worse, if the increase token expires and the RM rolls back 
the allocation of the increase part of the request, we end up with a partially 
fulfilled request, as we are not able to rollback the decrease part of the 

IMHO, it is much cleaner to clearly separate increase and decrease requests at 
the user API level. If a user wants to increase cpu and decrease memory, he 
should send out two separate requests. Thoughts?

bq. So how about {code}public abstract void 
onContainersResourceChanged(Map<Container,Container> oldToNewContainers); 
{code} OR {code}public abstract void 

I thought about providing the old containers in the callback method. Right now 
{{AMRMClientImpl}} remembers old containers in the {{pendingChange}} map, but 
the problem is, in the {{AMRMClientImpl.allocate}} call, once an 
increase/decrease approval is received, the old containers are immediately 
removed from the pending map. So by the time the {{AMRMClientAsyncImpl}} 
callback handler thread starts to process the response, the old containers 
won't be there any more:
+        if (!pendingIncrease.isEmpty() && 
!allocateResponse.getIncreasedContainers().isEmpty()) {
removePendingChangeRequests(allocateResponse.getDecreasedContainers(), true);
+        }
+        if (!pendingDecrease.isEmpty() && 
!allocateResponse.getDecreasedContainers().isEmpty()) {
removePendingChangeRequests(allocateResponse.getDecreasedContainers(), false);
+        }
My thought is, since we already ask the user to provide the old container when 
he sends out the change request, he should have the old container already, so 
we don't necessarily have to provide the old container info in the callback 
method. Thoughts?

bq. Would there be a case (maybe not currently) when a change container request 
can fail on the RM? Should the callback allow notifying about a failure to 
change the container?
The {{AbstractCallbackHandler.onError}} will be called when the change 
container request throws exception on the RM side.

bq. What is the RM notifies AMRMClient about a container completed. That 
container happens to have a pending change request? What should happen in this 
case? Should the AMRM client clear that pending request? Should it also notify 
the user that pending container change request has failed or just rely on 
onContainerCompleted() to let the AM get that information.
I think in this case AMRMClient should clear all pending requests that belong 
to this container. I will add that logic in. Thanks!

bq. I would be wary of overloading cancel with a second container change 
request. To be clear, here we are discussing user facing semantics and API. 
Having clear semantics is important vs implicit or overloaded behavior.
I am not against providing a separate cancel API. But I think the API needs to 
be clear that the cancel is only for increase request, NOT decrease request 
(just like we don't have something like cancel release container). For example, 
 we can have something like the following. Thoughts?
  public abstract void cancelContainerResourceIncrease(Container container)

bq. Is there an existing test for that code path that could be augmented to 
make sure that the new changes are tested?

I didn't find existing tests that test the pending list on RM restart, I will 
try to add a test case for that. Thanks.

> Make AMRMClient support send increase container request and get 
> increased/decreased containers
> ----------------------------------------------------------------------------------------------
>                 Key: YARN-1509
>                 URL: https://issues.apache.org/jira/browse/YARN-1509
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: resourcemanager
>            Reporter: Wangda Tan (No longer used)
>            Assignee: MENG DING
>         Attachments: YARN-1509.1.patch, YARN-1509.2.patch, YARN-1509.3.patch, 
> YARN-1509.4.patch, YARN-1509.5.patch
> As described in YARN-1197, we need add API in AMRMClient to support
> 1) Add increase request
> 2) Can get successfully increased/decreased containers from RM

This message was sent by Atlassian JIRA

Reply via email to