[ 
https://issues.apache.org/jira/browse/YARN-4129?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14951458#comment-14951458
 ] 

Sangjin Lee commented on YARN-4129:
-----------------------------------

Sorry [~Naganarasimha] it took me a while to review this.

I'm generally +1 on the effort here to reduce unnecessary layers (getting rid 
of event types) and additional flexibility you mentioned.

I know also there is a discussion on whether we should set createdTime, 
modifiedTime, etc. on the entities themselves (forgot the JIRA id), and it has 
some implications on this. I will chime in there later, but IMO it'd be good to 
set things like createdTime directly on the entities to have consistent and 
uniform access to those important times. We can make those changes (if we 
agree) in that JIRA, though.

(ResourceManager.java)
- l.396: the service is being added twice (another in l.275); I would say 
remove l.396
- l.514: I'm slightly confused that (apart from l.396) the publisher is 
registered once to the RM itself and another time here to the RMActiveServices. 
Is it needed? How would the service stop work (since these are composite 
services)?

(SystemMetricsPublisher.java)
- l.27: nit: space before the brace

(TimelineServiceV2Publisher.java)
- l.80: normally we call {{super.serviceStart()}} at the end rather than at the 
beginning, right?
- l.155: Are you referring to the issue of having the app level timeline 
collector hanging around to process late-coming writes? If so, we should add 
the JIRA id here in the comment so we that we can keep track. If not, could you 
please explain the TODO comment here?


> Refactor the SystemMetricPublisher in RM to better support newer events
> -----------------------------------------------------------------------
>
>                 Key: YARN-4129
>                 URL: https://issues.apache.org/jira/browse/YARN-4129
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: resourcemanager
>            Reporter: Naganarasimha G R
>            Assignee: Naganarasimha G R
>         Attachments: YARN-4129.YARN-2928.001.patch
>
>
> Currently to add new timeline event/ entity in RM side, one has to add a 
> method in publisher and a method in handler and create a new event class 
> which looks cumbersome and redundant. also further all the events might not 
> be required to be published in V1 & V2. So adopting the approach similar to 
> what was adopted in YARN-3045(NM side)



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to