Sangjin Lee commented on YARN-4058:

I see. We might then want to address it on our branch. I would still be 
reluctant to touch {{ApplicationACLsManager}} or even the constructor. We would 
end up touching quite a few files just to do that clean-up. For that issue, I'd 
raise it for the trunk and have it fixed there.

As for the application creation issue, I have one small nit, and one larger 
question to discuss. As for the nit,

901               if (null == 
902                   application)) {

This style (having values like null first and then the expression) is a 
carry-over from the C-style defensive coding, and is no longer needed in java. 
I would just do the normal style:

901               if (context.getApplications().putIfAbsent(applicationID,
902                   application) == null) {

I know it's existing code, but it'd be good to change that.

The larger question is this. The timeline client is being created *and started* 
as part of the {{ApplicationImpl}} constructor call. However, it seems like 
we're not stopping the timeline client anywhere. Right now, it might not be a 
big issue as the timeline client isn't doing anything special during the 
service start, but what if we create things like thread pools as part of the 
timeline client as you suggested? Then the timeline client must be stopped 
properly as part of lifecycle management. It's not clear how we can do that if 
they are created inside {{ApplicationImpl}}. Any thoughts on that?

> Miscellaneous issues in NodeManager project
> -------------------------------------------
>                 Key: YARN-4058
>                 URL: https://issues.apache.org/jira/browse/YARN-4058
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Naganarasimha G R
>            Assignee: Naganarasimha G R
>            Priority: Minor
>         Attachments: YARN-4058.YARN-2928.001.patch
> # TestSystemMetricsPublisherForV2.testPublishApplicationMetrics is failing 
> # Unused ApplicationACLsManager in ContainerManagerImpl
> # In ContainerManagerImpl.startContainerInternal ApplicationImpl instance is 
> created and then checked whether it exists in context.getApplications(). 
> everytime ApplicationImpl is created state machine is intialized and 
> TimelineClient is created which is required only if added to the context.

This message was sent by Atlassian JIRA

Reply via email to