[ https://issues.apache.org/jira/browse/YARN-3505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14534850#comment-14534850 ]
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(), updateLogAggregationDiagnosticMessages(), updateLogAggregationFailureMessages(), etc.? In addition, what would happen if (this.logAggregationSucceed + this.logAggregationFailed) != this.logAggregationStatus.size()? {code} private static int MAX_LOG_AGGREGATION_DIAGNOSTICS_IN_MEMORY = 10; {code} Shall we make it configurable? May be as private first (not putting on yarn-default.xml)? In AppBlock.java, {code} + protected LogAggregationStatus getLogAggregationStatus() { + return null; + } {code} Can we add a comment to said it has to be overriden in subclass? {code} + th(_TH, "Last 10 Diagnostis Messages"). {code} 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 (v6.3.4#6332)