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

Zhijie Shen commented on YARN-2581:
-----------------------------------

It looks good to me overall. Just some nits. 

1. Can't we let this.logAggregationContext = logContextData?
{code}
+      LogAggregationContextPBImpl logContextData =
+          new LogAggregationContextPBImpl(
+            LogAggregationContextProto.parseFrom(bytes));
+      this.logAggregationContext =
+          LogAggregationContext.newInstance(logContextData.getIncludePattern(),
+            logContextData.getExcludePattern(),
+            logContextData.getRollingIntervalSeconds());
{code}

2. Can the following
{code}
+    Assert.assertEquals(returned.getIncludePattern(), "includePattern");
+    Assert.assertEquals(returned.getExcludePattern(), "excludePattern");
+    Assert.assertTrue(returned.getRollingIntervalSeconds() == interval);
{code}
be changed to
{code}
+    Assert.assertEquals("includePattern", returned.getIncludePattern());
+    Assert.assertEquals("excludePattern", returned.getExcludePattern());
+    Assert.assertEquals(interval, returned.getRollingIntervalSeconds());
{code}
Though it makes no difference when assertion passes, it could show accurate 
error when assertion fails.

[~vinodkv], do you want to have a look at it too?

> NMs need to find a way to get LogAggregationContext
> ---------------------------------------------------
>
>                 Key: YARN-2581
>                 URL: https://issues.apache.org/jira/browse/YARN-2581
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager, resourcemanager
>            Reporter: Xuan Gong
>            Assignee: Xuan Gong
>         Attachments: YARN-2581.1.patch, YARN-2581.2.patch, YARN-2581.3.patch
>
>
> After YARN-2569, we have LogAggregationContext for application in 
> ApplicationSubmissionContext. NMs need to find a way to get this information.
> We have this requirement: For all containers in the same application should 
> honor the same LogAggregationContext.



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

Reply via email to