Naganarasimha G R commented on YARN-3044:

bq. Sorry to put my comments at last minute.
No probs, better late than never :)

bq. 1. I incline to only having ContainerEntity, but RM and NM may put 
different info/event about it based on their knowledge.
+1 for different event, this would be sufficient to capture difference in time 
being displayed when published from RM and NM(earlier we were having different 
container entity for this reason.). [~djp] please inform if anything else can 
differ if published from RM and NM which needs to be captured seperately.

bq. 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 
May be i did not get this clearly, but AFAIK the packages and classes for the 
Timeline events & entities are different and the way we publish entities is 
also different, so though the code looks duplicated i think nutting further to 
be reduced/cleaned up here. 

bq. 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. 
Well i had raised config related queries earlier as it dint get concluded was 
planning to get it done as part of a new jira, AFAIK intention in ATS is to 
collect the App side configs than server side ones. And RM will not be aware of 
App configs, so my initial idea was to support additional interface in the 
client to publish Application specific configs. Correct me if i am wrong  and 
also inform whether its ok to handle configs in another jira. 

 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 -> 
5. Methods/innner classes in SystemMetricsPublisher don't need to be changed to 
"public". Default is enough to access them?
will get these corrected. 

bq. 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.
{{yarn.system-metrics-publisher.rm.publish.container-metrics}} has been 
additionally added just to ensure container life cycle metrics are not emitted 
always from RM and only if required we will publishing it.
Initially in YARN-3034 
 we wanted to proceed with having single config like 
{{yarn.system-metrics-publisher.enabled}} (as existing one 
{{yarn.resourcemanager.system-metrics-publisher.enabled}} was specific to RM) 
and have {{yarn.timeline-service.version}}
but you had 
 to have single config {{yarn.system-metrics-publisher.enabled}} and hence i 
had remodified.

> [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