Li Lu commented on YARN-3390:

Hi [~zjshen], thanks for the patch! Here are some of my comments. Most of them 
are quite minor:

# Changes in RMContainerAllocator.java appears to be irrelevant. Seems like 
this is changed by an IDE by mistake (on a refactoring)?
# In the following lines
+    for (String tag : app.getApplicationTags()) {
+      String value = null;
+      if ((value = getFlowContext(TimelineUtils.FLOW_NAME_TAG_PREFIX, tag)) != 
the first null assignment to value is marked as redundant
+      if ((value = getFlowContext(TimelineUtils.FLOW_NAME_TAG_PREFIX, tag)) != 
+          && !value.isEmpty()) {
+          collector.getTimelineEntityContext().setFlowName(value);
+      } else if ((value = 
getFlowContext(TimelineUtils.FLOW_VERSION_TAG_PREFIX, tag)) != null
+          && !value.isEmpty()) {
+        collector.getTimelineEntityContext().setFlowVersion(value);
+      } else if ((value = getFlowContext(TimelineUtils.FLOW_RUN_ID_TAG_PREFIX, 
tag)) != null
+          && !value.isEmpty()) {
+        collector.getTimelineEntityContext().setFlowRunId(Long.valueOf(value));
+      }
Maybe we’d like to use a switch statement to deal with this? We may first split 
the tag into two parts, based on the first “:”, and then switch the first part 
of the returned array to set the second part of the array into flow name, 
version, and run id. Am I missing any fundamental obstacles for us to do this 
here? (String switch is available from Java 7)
# Rename {{MyNMTimelineCollectorManager}} in 
TestTimelineServiceClientIntegration with something indicating it's for testing?
# In the following lines:
-  protected TimelineCollectorContext getTimelineEntityContext() {
+  public TimelineCollectorContext getTimelineEntityContext() {
We're exposing TimelineCollectorContext but we're not annotating the class. 
Even though we may treat unannotated classes as Audience.Private, maybe we'd 
like to mark it as unstable?
# In TimelineCollectorManager, I'm still having this question, although we may 
not want to address it in this JIRA: are there any special consistency 
requirements that prevent us from using ConcurrentHashMap?
# In TimelineCollectorWebService, why we're removing the utility function 
{{getCollector}}? I think we can reuse it when adding new web services. 

> 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
> 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