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

Junping Du commented on YARN-3437:
----------------------------------

Thanks [~sjlee0] for updating the patch! 
The latest patch looks good in overall, some minor comments:
{code}
+  public TimelineCollector putIfAbsent(ApplicationId appId,
+      TimelineCollector collector) {
+    String id = appId.toString();
+    TimelineCollector collectorInTable;
+    boolean collectorIsNew = false;
+    synchronized (collectors) {
+      collectorInTable = collectors.get(id);
+      if (collectorInTable == null) {
+        try {
+          // initialize, start, and add it to the collection so it can be
+          // cleaned up when the parent shuts down
+          collector.init(getConfig());
+          collector.start();
+          collectors.put(id, collector);
+          LOG.info("the collector for " + id + " was added");
+          collectorInTable = collector;
+          collectorIsNew = true;
+        } catch (Exception e) {
+          throw new YarnRuntimeException(e);
+        }
+      } else {
+        String msg = "the collector for " + id + " already exists!";
+        LOG.error(msg);
+      }
+    }
+
+    if (collectorIsNew) {
+      postPut(appId, collector);
+    }
+
+    return collectorInTable;
+  }
{code}
I understand this code piece is moved from other place. However, I think it 
need to be improved:
- For performance perspective, we should move LOG.info() out of synchronized 
block (may be move out of collector.start()?). 
- we don't need to LOG.ERROR (replace with INFO?) if collector exists, general 
semantic for putIfAbsent should allow put the same object in concurrent threads.

For remove(), similar that we should move collector.stop() and LOG.info() out 
of synchronized block.


> convert load test driver to timeline service v.2
> ------------------------------------------------
>
>                 Key: YARN-3437
>                 URL: https://issues.apache.org/jira/browse/YARN-3437
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Sangjin Lee
>            Assignee: Sangjin Lee
>         Attachments: YARN-3437.001.patch, YARN-3437.002.patch, 
> YARN-3437.003.patch
>
>
> This subtask covers the work for converting the proposed patch for the load 
> test driver (YARN-2556) to work with the timeline service v.2.



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

Reply via email to