Sangjin Lee commented on YARN-3334:

Sorry it took a while for me to go over the patch. I just had time now and here 
is my initial feedback.

- l.364: I would prefer the type as interfaces (Map instead of 
- getTimelineClient() could initialize multiple timeline client instances for a 
given app id (there’s a race): it could be addressed by moving the code that 
initializes to after putIfAbsent() succeeds
- l.500: I would also get rid of the containsKey() call; so something like the 
following might be good:
public TimelineClient getTimelineClient(ApplicationId id) {
  TimelineClient client = this.timelineClients.get(id);
  if (client == null) {
    client = TimelineClient.createTimelineClient(id);
    TimelineClient old = this.timelineClients.putIfAbsent(id, client);
    if (old != null) {
      client = old;
    } else {
  return client;

- I would prefer a method like removeClient() as opposed to returning the 
underlying map for modification via getTimelineClients()

- l.267: it’s usually a good practice to restore the interrupt upon handling an 
InterruptedException via Thread.interrupt(), however trivial it may be

- Sorry I might be missing something obvious, but why was this change necessary?

> [Event Producers] NM TimelineClient life cycle handling and container metrics 
> posting to new timeline service.
> --------------------------------------------------------------------------------------------------------------
>                 Key: YARN-3334
>                 URL: https://issues.apache.org/jira/browse/YARN-3334
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager
>    Affects Versions: YARN-2928
>            Reporter: Junping Du
>            Assignee: Junping Du
>         Attachments: YARN-3334-demo.patch, YARN-3334-v1.patch, 
> YARN-3334-v2.patch, YARN-3334-v3.patch
> After YARN-3039, we have service discovery mechanism to pass app-collector 
> service address among collectors, NMs and RM. In this JIRA, we will handle 
> service address setting for TimelineClients in NodeManager, and put container 
> metrics to the backend storage.

This message was sent by Atlassian JIRA

Reply via email to