Sangjin Lee commented on YARN-3390:

I'm very sorry for revisiting this one more time after saying LGTM. :) But it 
occurred to me while looking at your patch.

You brought up a good point that collector.init() and collector.start() can be 
done without synchronization after put() is done (based on the collectorIsNew 
flag). I now realize that it is exactly the same semantics as 
ConcurrentHashMap, and we should be able to do this much more simply using 
ConcurrentHashMap. Sorry for the late change, but I propose this:

private final ConcurrentMap<ApplicationId,TimelineCollector> collectors = new 
public TimelineCollector putIfAbsent(ApplicationId appId, TimelineCollector 
collector) {
  TimelineCollector collectorInTable = collectors.putIfAbsent(appId, collector);
  if (collectorInTable == null) {
    LOG.info("the collector for " + appId + " was added");
    collectorInTable = collector;
    // initialize, start, and add it to the collection so it can be
    // cleaned up when the parent shuts down
    postPut(appId, collectorInTable);
  } else {
    LOG.info("the collector for " + appId + " already exists!");
  return collectorInTable;

Other code remains the same. Essentially the synchronization block in the 
current patch collapses into a single call on CHM.putIfAbsent().

Previously, either [~djp] or [~gtCarrera9] pointed out that there may be a race 
condition in terms of getting different collectors if CHM is used, but looking 
at this, I don't see that happening. Thoughts?

> Reuse TimelineCollectorManager for RM
> -------------------------------------
>                 Key: YARN-3390
>                 URL: https://issues.apache.org/jira/browse/YARN-3390
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Zhijie Shen
>            Assignee: Zhijie Shen
>         Attachments: YARN-3390.1.patch, YARN-3390.2.patch, YARN-3390.3.patch
> RMTimelineCollector should have the context info of each app whose entity  
> has been put

This message was sent by Atlassian JIRA

Reply via email to