[jira] [Commented] (YARN-1376) NM need to notify the log aggregation status to RM through Node heartbeat

[Junping Du (JIRA)]

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

Junping Du commented on YARN-1376:
----------------------------------

Thanks [~xgong] for updating the patch.
I just reviewed and have some comments below, most are minor issues except the 
last one:
{code}
public static final long DEFAULT_LOG_AGGREGATION_STATUS_TIME_OUT_MS = 
10*60*1000;
{code}
Add space between number and *.

In LogAggregationReportPBImpl.java,
{code}
+  private LogAggregationStatus convertFromProtoFormat(
+      LogAggregationStatusProto s) {
+    return LogAggregationStatus.valueOf(s.name().replace(
+      LOGAGGREGATION_STATUS_PREFIX, ""));
+  }
+
+  private LogAggregationStatusProto
+      convertToProtoFormat(LogAggregationStatus s) {
+    return LogAggregationStatusProto.valueOf(LOGAGGREGATION_STATUS_PREFIX
+        + s.name());
+  }
{code}
Looks like we are adding/removing LOGAGGREGATION_STATUS_PREFIX between java obj 
and proto obj. I think this is not necessary? Am I missing something here?

In NodeStatusUpdaterImpl.java,
{code}
+      if (!latestLogAggregationReports.containsKey(logAggregationReport
+        .getApplicationId())) {
          ...  // A
+      } else {
         ... // B
       }
{code}
Can we remove ! in if condition and adjust sequence of A and B, which looks 
simpler?

In uploadLogsForContainers() of AppLogAggregatorImpl.java,
{code}
       } catch (Exception e) {
         LOG.error(
           "Failed to move temporary log file to final location: ["
               + remoteNodeTmpLogFileForApp + "] to ["
               + renamedPath + "]", e);
+        diagnosticMessage =
+            "Log uploaded failed for Application: " + appId
+                + " in NodeManager: "
+                + LogAggregationUtils.getNodeString(nodeId) + " at "
+                + Times.format(currentTime) + "\n";
       }
+
+      LogAggregationReport report =
+          Records.newRecord(LogAggregationReport.class);
+      report.setApplicationId(appId);
+      report.setNodeId(nodeId);
+      report.setDiagnosticMessage(diagnosticMessage);
+      if (appFinished) {
+        report.setLogAggregationStatus( LogAggregationStatus.FINISHED);
+      } else {
+        report.setLogAggregationStatus( LogAggregationStatus.RUNNING);
+      }
+      this.context.getLogAggregationStatusForApps().add(report);
{code}
Looks like we only set LogAggregationStatus to FINISHED or RUNNING here even it 
is failed to move temp log to HDFS. It doesn't seems correct to me. We should 
add a FAILED state for LogAggregationStatus to address this case?

Other looks fine to me.


> NM need to notify the log aggregation status to RM through Node heartbeat
> -------------------------------------------------------------------------
>
>                 Key: YARN-1376
>                 URL: https://issues.apache.org/jira/browse/YARN-1376
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Xuan Gong
>            Assignee: Xuan Gong
>         Attachments: Screen Shot 2015-04-07 at 9.30.42 AM.png, 
> YARN-1376.1.patch, YARN-1376.2.patch, YARN-1376.2.patch, 
> YARN-1376.2015-04-04.patch, YARN-1376.2015-04-06.patch, 
> YARN-1376.2015-04-07.patch, YARN-1376.3.patch, YARN-1376.4.patch
>
>
> Expose a client API to allow clients to figure if log aggregation is 
> complete. The ticket is used to track the changes on NM side



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