Jian He commented on YARN-1372:

Thanks for updating the patch. Some comments and naming suggestions:
- NodeStatusUpdater import changes only, we can revert
- indentation format of the second line.
  public void removeCompletedContainersFromContext(List<ContainerId>
                                                       containerIds) throws
  public RMNodeCleanedupContainerNotifiedEvent(NodeId nodeId,
                                               ContainerId contId) {
- why adding {{context.getContainers().remove(cid);}}  in 
removeVeryOldStoppedContainersFromContext method? won’t this remove the 
containers from context immediately when we send the container statuses across, 
which contradicts the rest of the changes?
- In NodeStatusUpdaterImpl, previousCompletedContainers cache is not needed any 
more, as we make NM remove containers from context only after it gets the 
notification. We can remove this; Instead, in 
NodeStatusUpdater#getContainerStatuses, while we are looping all the 
containers, we can check whether the corresponding application exists, if Not, 
remove it from context.
- make sure {{context.getNMStateStore().removeContainer(cid);}} is called after 
receiving the notification from RM as well.
- {{RMNodeEventType#CLEANEDUP_CONTAINER_NOTIFIED}}: put in a new section where 
source is RMAppAttempt. how about rename to FINISHED_CONTAINERS_PULLED_BY_AM; 
similarly RMNodeCleanedupContainerNotifiedEvent -> 
- In RMAppAttemptImpl#BaseFinalTransition, we can clear 
finishedContainersSentToAM, in case that AM unexpectedly crashes.
- I think Map<NodeId, List<ContainerStatus >> is more space efficient than: 
  private Map<ContainerStatus, NodeId> finishedContainersSentToAM =
      new HashMap<ContainerStatus, NodeId>();
- :  format convention is to have method body in a different line from the 
method head.
public NodeId getNodeId() { return this.nodeId; }
- RMNodeImpl#cleanupContainersNotified, may be rename to 
finishedContainersPulledByAM. similarly CleanedupContainerNotifiedTransition to 
- NodeHeartbeatResponse#addCleanedupContainersNotified, how about  
addFinishedContainersPulledByAM; similarly for the getter 
NodeHeartbeatResponse#getCleanedupContainersNotified and the proto file. Also 
add some code comments to explain why adding this new API.

> Ensure all completed containers are reported to the AMs across RM restart
> -------------------------------------------------------------------------
>                 Key: YARN-1372
>                 URL: https://issues.apache.org/jira/browse/YARN-1372
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: resourcemanager
>            Reporter: Bikas Saha
>            Assignee: Anubhav Dhoot
>         Attachments: YARN-1372.001.patch, YARN-1372.001.patch, 
> YARN-1372.002_NMHandlesCompletedApp.patch, 
> YARN-1372.002_RMHandlesCompletedApp.patch, 
> YARN-1372.002_RMHandlesCompletedApp.patch, YARN-1372.003.patch, 
> YARN-1372.prelim.patch, YARN-1372.prelim2.patch
> Currently the NM informs the RM about completed containers and then removes 
> those containers from the RM notification list. The RM passes on that 
> completed container information to the AM and the AM pulls this data. If the 
> RM dies before the AM pulls this data then the AM may not be able to get this 
> information again. To fix this, NM should maintain a separate list of such 
> completed container notifications sent to the RM. After the AM has pulled the 
> containers from the RM then the RM will inform the NM about it and the NM can 
> remove the completed container from the new list. Upon re-register with the 
> RM (after RM restart) the NM should send the entire list of completed 
> containers to the RM along with any other containers that completed while the 
> RM was dead. This ensures that the RM can inform the AM's about all completed 
> containers. Some container completions may be reported more than once since 
> the AM may have pulled the container but the RM may die before notifying the 
> NM about the pull.

This message was sent by Atlassian JIRA

Reply via email to