[jira] [Commented] (YARN-3347) Improve YARN log command to get AMContainer logs as well as running containers logs

["Junping Du (JIRA)" <ji...@apache.org>]

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

Junping Du commented on YARN-3347:
----------------------------------

Hi [~xgong], thanks for reporting this issue and delivering a patch for fixing this! 
This looks like a very helpful feature for trouble shooting. I went through the patch quickly and have some comments so far:
{code}
+    Option amOption = new Option(AM_CONTAINER_OPTION, true, 
+      "Prints the AM Container logs for this application. "
+      + "Specify comma-separated value to get logs for related AM Container. "
+      + "To get logs for all AM Containers, use -am ALL. "
+      + "To get logs for the latest AM Container, use -am -1. "
+      + "By default, it will only print out syslog. Work with -logFiles "
+      + "to get other logs");
{code}
For comma-separated value, do we mean attempt number? If so, may be we should describe more explicitly here? Also, can we use 0 (instead of -1) for AM container of latest attempt. If so, all negative value here is illegal. 

{code}
+        if (getConf().getBoolean(YarnConfiguration.APPLICATION_HISTORY_ENABLED,
+          YarnConfiguration.DEFAULT_APPLICATION_HISTORY_ENABLED)) {
+          System.out.println("Please enable the application history service. Or ");
+        }
{code}
Missing ! before getConf()?

In method of printAMContainerLogsForRunningApplication(),
{code}
+    boolean printAll = amContainers.contains("ALL");
+
+    for (int i = 0; i < amContainersInfo.length(); i++) {
+      boolean printThis = amContainers.contains(Integer.toString(i+1)) 
+          || (i == (amContainersInfo.length()-1)
+              && amContainers.contains(Integer.toString(-1)));
+      if (printAll || printThis) {
+        String nodeHttpAddress =
+            amContainersInfo.getJSONObject(i).getString("nodeHttpAddress");
+        String containerId =
+            amContainersInfo.getJSONObject(i).getString("containerId");
+        String nodeId = amContainersInfo.getJSONObject(i).getString("nodeId");
+        if (nodeHttpAddress != null && containerId != null
+            && !nodeHttpAddress.isEmpty() && !containerId.isEmpty()) {
+          printContainerLogsFromRunningApplication(conf, appId, containerId,
+            nodeHttpAddress, nodeId, logFiles, logCliHelper, appOwner);
+        }
+      }
+    }
+    return 0;
+  }
{code}
Sounds like we are re-order the sequence of user's input which seems unnecessary to me. I would suggest to keep order from user's input or it could confuse people. Also, the logic here sounds not quite straightforward. I would expect something simper, like pseudo code below:
{code}
if (printAll) {
  go through amContainersInfo and print
}
for (amContainer : amContainers) {
  amContainer == -1? print amContainersInfo(last-one) : 
  print amContainersInfo(amContainer -1);
}
{code}

Also, for method of run(String[] args), it looks very complexity for now. Can we do some refactor work there and put some comments inline?

> Improve YARN log command to get AMContainer logs as well as running containers logs
> -----------------------------------------------------------------------------------
>
>                 Key: YARN-3347
>                 URL: https://issues.apache.org/jira/browse/YARN-3347
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: log-aggregation
>            Reporter: Xuan Gong
>            Assignee: Xuan Gong
>         Attachments: YARN-3347.1.patch, YARN-3347.1.rebase.patch, YARN-3347.2.patch, YARN-3347.2.rebase.patch
>
>
> Right now, we could specify applicationId, node http address and container ID to get the specify container log. Or we could only specify applicationId to get all the container logs. It is very hard for the users to get logs for AM container since the AMContainer logs have more useful information. Users need to know the AMContainer's container ID and related Node http address.
> We could improve the YARN Log Command to allow users to get AMContainer logs directly



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