Junping Du commented on YARN-1402:

Thanks [~xgong] for updating the patch!
bq. No need to do it. We only call getLogAggregationStatusForAppReport() if 
Sync offline with Xuan that if we update LogAggregationStatus of an app in 
every node's LA report, that would take race condition for status update of 
different node report so additional lock could be needed which is completely 
unnecessary. So I am good with current way. 

Some other minor comments for updated patch:
+public enum LogAggregationStatus {
We should document semantics for LogAggregationStatus somewhere, at least for 
FAILED and TIME_OUT, or other developers could get confused here.

+  private static final String LOGAGGREGATION_STATUS_PREFIX = "LOG_";

this.logAggregationStatusForAppReport== LogAggregationStatus.FAILED
add a space between logAggregationStatusForAppReport and ==

For getLogAggregationStatusForAppReport(), we should add some comments that why 
we didn't failed early (if app hasn't been completed) for the reason we 
discussed offline.

+          case NOT_START:
+            logNotStartCount ++;
+            logCompletedCount ++;
Omit the space between logNotStartCount and ++ (and for some other places) to 
conform general code conversion.

Also, there is one important thing we need to think seriously here: do we need 
to cleanup nodes' log aggregation reports in RMApps? I am afraid we have to do 
it because RMApp will always stay in RMContext. Also, each RMApp could contains 
thousands of node reports in large cluster which could occupy so much memory of 
Resource Manager. Thoughts?

> Related Web UI, CLI changes on exposing client API to check log aggregation 
> status
> ----------------------------------------------------------------------------------
>                 Key: YARN-1402
>                 URL: https://issues.apache.org/jira/browse/YARN-1402
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Xuan Gong
>            Assignee: Xuan Gong
>         Attachments: YARN-1402.1.patch, YARN-1402.2.patch, 
> YARN-1402.3.1.patch, YARN-1402.3.2.patch, YARN-1402.3.patch

This message was sent by Atlassian JIRA

Reply via email to