Zhijie Shen commented on YARN-3044:

Sorry to put my comments at last minute:

1. I'm still not sure why it is necessary to have RMContainerEntity. Whether 
the container entity comes from RM or NM, it's about container's info. Any 
reason we want differentiate both? At reader side, if I want to list all 
containers of an app, should I return RMContainerEntity or ContainerEntity? I 
incline to only having ContainerEntity, but RM and NM may put different 
info/event about it based on their knowledge.

2. Should v1 and v2 publisher only differentiate at publishXXXXEvent, however, 
it seems that we duplicate code more than that. And perhaps defining and 
implementing SystemMetricsEvent.toTimelineEvent can further cleanup the code.

3. I saw v2 is going to send config, but where the config is coming from. Did 
we conclude who and how to send the config? IAC, sending config seems to be 
half done. And we can use {{entity.addConfigs(event.getConfig());}}. No need to 
iterate over config collection and put each config one-by-one.

4. yarn.system-metrics-publisher.rm.publish.container-metrics -> 
374       public static final String RM_PUBLISH_CONTAINER_METRICS_ENABLED = 
375           + "system-metrics-publisher.rm.publish.container-metrics";
376       public static final boolean 
377           false;
Moreover, I also think we should  not have 
"yarn.system-metrics-publisher.enabled" too, and reuse the existing config. And 
it's not limited to RM metrics publisher, but all existing ATS service. IMHO, 
the better practice is to reuse the existing config. And we can have a global 
config (or env var) timeline-service.version to determine the service is 
enabled with v1 or v2 implementation. Anyway, it's a separate problem, I'll 
file a separate jira for it.

5. Methods/innner classes in SystemMetricsPublisher don't need to be changed to 
"public". Default is enough to access them? 

> [Event producers] Implement RM writing app lifecycle events to ATS
> ------------------------------------------------------------------
>                 Key: YARN-3044
>                 URL: https://issues.apache.org/jira/browse/YARN-3044
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Sangjin Lee
>            Assignee: Naganarasimha G R
>              Labels: BB2015-05-TBR
>         Attachments: YARN-3044-YARN-2928.004.patch, 
> YARN-3044-YARN-2928.005.patch, YARN-3044-YARN-2928.006.patch, 
> YARN-3044-YARN-2928.007.patch, YARN-3044.20150325-1.patch, 
> YARN-3044.20150406-1.patch, YARN-3044.20150416-1.patch
> Per design in YARN-2928, implement RM writing app lifecycle events to ATS.

This message was sent by Atlassian JIRA

Reply via email to