Mailing-List: contact yarn-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: yarn-issues@hadoop.apache.org
Date: Mon, 4 Nov 2013 18:34:19 +0000 (UTC)
From: "Zhijie Shen (JIRA)" <jira@apache.org>
To: yarn-issues@hadoop.apache.org
Message-ID: <JIRA.12659994.1374829121386.17276.1383590059673@arcas>
In-Reply-To: <JIRA.12659994.1374829121386@arcas>
References: <JIRA.12659994.1374829121386@arcas>
Subject: [jira] [Commented] (YARN-979) [YARN-321] Add more APIs related to
 ApplicationAttempt and Container in ApplicationHistoryProtocol
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


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

Zhijie Shen commented on YARN-979:
----------------------------------

The patch is almost good with the following minor issues:

* The following javadoc is inconsistent with ApplicationAttemptReport (YARN-978)
{code}
+   * <li>host - set to "N/A"</li>
+   * <li>RPC port - set to -1</li>
+   * <li>client token - set to "N/A"</li>
+   * <li>diagnostics - set to "N/A"</li>
+   * <li>tracking URL - set to "N/A"</li>
{code}

* As is mentioned in the other two jiras, please move "GetApplicationAttemptReportRequestProtoOrBuilder p = viaProto ? proto : builder;" later.
{code}
+  @Override
+  public ApplicationAttemptId getApplicationAttemptId() {
+    GetApplicationAttemptReportRequestProtoOrBuilder p
+        = viaProto ? proto : builder;
+    if (this.applicationAttemptId != null) {
+      return this.applicationAttemptId;
+    }
+    if (!p.hasApplicationAttemptId()) {
+      return null;
+    }
+    this.applicationAttemptId =
+        convertFromProtoFormat(p.getApplicationAttemptId());
+    return this.applicationAttemptId;
+  }
{code}

* You need to change hadoop-yarn-api/pom.xml to make application_history_client.proto to be compiled.

In addition to the patch's issues, I'd like to raise one design issue here, projecting some future problems. This patch makes different APIs for application/attempt/container, which is going to be a super set of the APIs of ApplicationClientProtocol. Now it's OK if we restrict our problem with the AHS domain. However, probably in the future, we'd like to integrate the ApplicationHistoryProtocol with ApplicationClientProtocol. In other word, from the view of users, they may inquiry any application use a client, which makes it transparent whether the application report is received via ApplicationClientProtocol if the application is running or via ApplicationHistoryProtocol if it is done. Then, ApplicationClientProtocol's and ApplicationHistoryProtocol's APIs mismatch. Users can inquiry finished attempts/containers, but not the running ones. ApplicationClientProtocol may need to add the APIs for attempt/container as well.

In addition, another choice of the API design is to still have the only getApplicationReport(), but have the options to load all attempts/containers reports or not. Just think it out aloud. Personally, I incline to the current API design, which is more flexible, but I'm a bit concerned about the future integration. Thoughts?

> [YARN-321] Add more APIs related to ApplicationAttempt and Container in ApplicationHistoryProtocol
> --------------------------------------------------------------------------------------------------
>
>                 Key: YARN-979
>                 URL: https://issues.apache.org/jira/browse/YARN-979
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Mayank Bansal
>            Assignee: Mayank Bansal
>         Attachments: YARN-979-1.patch, YARN-979-3.patch, YARN-979-4.patch, YARN-979.2.patch
>
>
> ApplicationHistoryProtocol should have the following APIs as well:
> * getApplicationAttemptReport
> * getApplicationAttempts
> * getContainerReport
> * getContainers
> The corresponding request and response classes need to be added as well.


--
This message was sent by Atlassian JIRA
(v6.1#6144)