Zhijie Shen commented on YARN-3039:

[~djp], thanks for updating the patch. It looks good to me overall. Some more 
minor comments about the patch.

1. No need to change the constructor and have newTimelineService flag. 
{{createTimelineClient(ApplicationId appId) {}} is the new one added for TS v2. 
appId shouldn't be null for TS v2. Otherwise the request won't reach the right 
app-level aggregator. We can based on this var to determine if working with new 
or old timeline service.
 public static TimelineClient newInstance() {
56          return new TimelineClientImpl();
57        }
59        @Public
60        public static TimelineClient newInstance(ApplicationId appId) {
61          return new TimelineClientImpl(appId);
62        }
64        @Public
65        public static TimelineClient newInstance(ApplicationId appId,
66            boolean newTimelineService) {
67          return new TimelineClientImpl(appId, newTimelineService);

2. New put method won't reach this method. It's only called by old put 
entity/domain operations. So no change is requited here
618       @Private
619       @VisibleForTesting
620       public ClientResponse doPostingObject(Object object, String path) {
And change
306         // TODO need to cleanup filter retry later.
307         //client.addFilter(retryFilter);
if (contextAppId == null) client.addFilter(retryFilter);
such that old put operation will still use the old retry logic, while the new 
one use the new retry logic.

3. Is the change in TestContainerLaunchRPC related to this jira? And TestRPC -> 
TestNMRPC or keept TestRPC in common and create a TestNMRPC in server-common to 
test server-side protocol?

4. You can remove the \@Public \@Private annotation around the server api stuff 
like ResourceTracker, or mark them Private.

5. {{void removeKnownAggregator(ApplicationId appId)}} seems not to be 
necessarily in context, and we can call Context. 
getKnownAggregators.remove(appId) to remove the entry. It seems that other 
collections in context are working in the same way.

> [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, 
> YARN-3039-v6.patch, YARN-3039-v7.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

Reply via email to