Junping Du commented on YARN-3505:

Thanks Xuan for updating the patch! The latest patch looks much closer. 

bq. Why we need to differentiate diagnosticMessage with failureMessages in 
LogAggregationReport? If we don't want to cache successful report, we can 
differentiate from LogAggregationStatus. Isn't it?
It seems this previous comment haven't been responsed so far. Just think it 
again, I think our intention is trying to differentiate the normal 
diagnosticMessage and errorMessages, so it should be fine to track 
LogAggregationDiagnosticsForNMs and LogAggregationFailureMessagesForNMs 
separatedly in RMAppImpl. However, I think we don't need this differentiation 
in LogAggregationReport as in case of FAILED case, diagnosticMessage and 
failureMessages store the same message which is duplicated. Isn't it?

Other minor comments:

In RMAppImpl.java, 
aggregateLogReport() method sounds too long and need some refactor work here. 
Can we abstract some methods like: updateLogAggregationStatus(), 
updateLogAggregationFailureMessages(), etc.? In addition, what would happen if 
(this.logAggregationSucceed + this.logAggregationFailed) != 

Shall we make it configurable? May be as private first (not putting on 

In AppBlock.java,
+  protected LogAggregationStatus getLogAggregationStatus() {
+    return null;
+  }
Can we add a comment to said it has to be overriden in subclass?

+        th(_TH, "Last 10 Diagnostis Messages").
Typo, "Diagnostis" => "Diagnostic"

> Node's Log Aggregation Report with SUCCEED should not cached in RMApps
> ----------------------------------------------------------------------
>                 Key: YARN-3505
>                 URL: https://issues.apache.org/jira/browse/YARN-3505
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: log-aggregation
>    Affects Versions: 2.8.0
>            Reporter: Junping Du
>            Assignee: Xuan Gong
>            Priority: Critical
>         Attachments: YARN-3505.1.patch, YARN-3505.2.patch, 
> YARN-3505.2.rebase.patch
> Per discussions in YARN-1402, we shouldn't cache all node's log aggregation 
> reports in RMApps for always, especially for those finished with SUCCEED.

This message was sent by Atlassian JIRA

Reply via email to