Junping Du commented on YARN-3045:

bq. The first patch was put up in April and later versions of the patch seem to 
have been +1'ed already by several folks. If there is a fundamental problem 
with the patch, we should address it.
I feel sorry for the long delay to get in this patch. Part of reason, I think 
is due to my review work on this JIRA get intermittently as other JIRA work and 
personal reasons. I appreciate [~Naganarasimha]'s patient on this JIRA's work 
and I am sure the latest patch (08) is getting much closer. One thing I need to 
clarify here is: from the history of above comments, there is no +1 on the 
patch before, but only several +1s on ideas.

bq. It would be good to keep the comments here really focused on this patch and 
open separate jiras for separate topics to that we can keep making progress.
Agree. We separated several topics out already.

bq. Sorry but it's not clear what the 2 options are. Could you kindly rephrase 
the options?
I think for the two option, Naga means : 1. make resource localization events 
wrap as application entity; 2. make resource localization events wrap as NM 
entity (in case we add it in future).

bq. Are some of these events already existing container events? If so, they 
shouldn't be repeated as application events redundantly, right? What would be 
the application-specific events that are not captured by container events?
ContainerEvent has more details about life cycle events than ApplicationEvent, 
i.e. RESOURCE_LOCALIZED, RESOURCE_FAILED, etc. Also, even for some duplicated 
event, like APPLICATION_CONTAINER_FINISHED in ApplicationEvent, ContainerEvent 
could provide more details: CONTAINER_EXITED_WITH_SUCCESS, 
some of these events are really trivial to application and that's why I push 
important levels from the beginning (so we can ignore them through 

Back to the 08 patch, mostly looks good to me but two small issues:
+/*        nmMetricsPublisher.containerCreated(container,
+            System.currentTimeMillis());*/
+        /*container.getNMTimelinePublisher().containerFinished(container,
+            System.currentTimeMillis());*/
Omit these codes which are not useful now.

For ApplicationEventDispatcher, ContainerEventDispatcher,  
LocalizationEventDispatcher: first, the name is a little confusing here: we 
should use *Handler rather than *Dispatcher as the functionality is handler of 
event but not dispatcher; second, for unrecognized event, we should log a warn 
message (at least debug message) instead of do nothing.

> [Event producers] Implement NM writing container lifecycle events to ATS
> ------------------------------------------------------------------------
>                 Key: YARN-3045
>                 URL: https://issues.apache.org/jira/browse/YARN-3045
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Sangjin Lee
>            Assignee: Naganarasimha G R
>         Attachments: YARN-3045-YARN-2928.002.patch, 
> YARN-3045-YARN-2928.003.patch, YARN-3045-YARN-2928.004.patch, 
> YARN-3045-YARN-2928.005.patch, YARN-3045-YARN-2928.006.patch, 
> YARN-3045-YARN-2928.007.patch, YARN-3045-YARN-2928.008.patch, 
> YARN-3045.20150420-1.patch
> Per design in YARN-2928, implement NM writing container lifecycle events and 
> container system metrics to ATS.

This message was sent by Atlassian JIRA

Reply via email to