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

Sangjin Lee commented on YARN-3039:
-----------------------------------

[~djp], I went over patch v.5 one more time, and it looks mostly good. I did 
have a few comments (as follows):

- TimelineClient.java
  -- l.59-64: I would vote for simply removing the method
  -- l.413: nit: space
  -- l.416: should we throw a RuntimeException? how about wrapping it in a 
YarnException?
- yarn-default.xml
  -- it still has 20 as the thread count default
- I’m not 100% certain that there is an issue, but I think we need to be 
careful about always removing registered/known aggregators when the app is 
done. Failure to do so would quickly ramp up the NM memory, and it would become 
unusable. Again, I _think_ the current code will properly remove a finished app 
from registered/known aggregator maps. It's just a reminder to be careful about 
this.

> [Aggregator wireup] Implement ATS app-appgregator service discovery
> -------------------------------------------------------------------
>
>                 Key: YARN-3039
>                 URL: https://issues.apache.org/jira/browse/YARN-3039
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Sangjin Lee
>            Assignee: Junping Du
>         Attachments: Service Binding for applicationaggregator of ATS 
> (draft).pdf, Service Discovery For Application Aggregator of ATS (v2).pdf, 
> YARN-3039-no-test.patch, YARN-3039-v2-incomplete.patch, 
> YARN-3039-v3-core-changes-only.patch, YARN-3039-v4.patch, YARN-3039-v5.patch
>
>
> Per design in YARN-2928, implement ATS writer service discovery. This is 
> essential for off-node clients to send writes to the right ATS writer. This 
> should also handle the case of AM failures.



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

Reply via email to