Mailing-List: contact yarn-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: yarn-issues@hadoop.apache.org
Date: Thu, 23 Oct 2014 01:23:34 +0000 (UTC)
From: "Zhijie Shen (JIRA)" <jira@apache.org>
To: yarn-issues@hadoop.apache.org
Message-ID: <JIRA.12723560.1403671800000.320104.1414027414567@Atlassian.JIRA>
In-Reply-To: <JIRA.12723560.1403671800000@Atlassian.JIRA>
References: <JIRA.12723560.1403671800000@Atlassian.JIRA>
 <JIRA.12723560.1403671800816@arcas>
Subject: [jira] [Commented] (YARN-2209) Replace AM resync/shutdown command
 with corresponding exceptions
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


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

Zhijie Shen commented on YARN-2209:
-----------------------------------

bq. Given that we already broken compatibility for rolling upgrades, the patch should be fine in that sense.

Hm... Make sense. The patch is good overall. Just some minor comments about it.

1. Use ApplicationAttemptNotFoundException instead of ApplicationNotFoundException?

2. Node need to suppress the each method if you already suppress the class.
{code}
  @SuppressWarnings("deprecation")
  public synchronized AMCommand getAMCommand() {
{code}

3. Is the change in ResourceCalculator.java related?

4. I saw deprecation warning around "import org.apache.hadoop.yarn.api.records.AMCommand;" in TestAllocateResponse. Is this the reason for jenkins -1 javac?

5. *H*eartbeats, to be consistent with other constant strings.
{code}
    public static final String AM_ALLOCATE = "App Master heartbeats";
{code}

6. Instead of "Assert.assertTrue(catchException);", why not "Assert.fail()" directly after "ar = am.allocate("h1", 1000, request, new ArrayList<ContainerId>());" Same for the following assertions.
{code}
    boolean catchException = false;
    try {
      ar = am.allocate("h1", 1000, request, new ArrayList<ContainerId>());
    } catch (ApplicationMasterNotRegisteredException e) {
      catchException = true;
    }
    Assert.assertTrue(catchException);
{code}

7. Unused import in TestAMRMRPCResponseId.java
{code}
Throwable exception = null;
    try {
      allocate(attempt.getAppAttemptId(), allocateRequest);
    } catch (Exception e) {
      exception = e.getCause();
    }
    Assert
      .assertTrue(exception instanceof InvalidApplicationMasterRequestException);
{code}
can be changed to
{code}
    try {
      allocate(attempt.getAppAttemptId(), allocateRequest);
      Assert.fail()
    } catch (Exception e) {
      Assert.assertTrue(e instanceof InvalidApplicationMasterRequestException);
    }
{code}

8. No need to change to split a single statement into the following two.
{code}
        allocateResponse = allocate(progressIndicator);
        return allocateResponse;
{code}

9. Unnecessary import in AMRMClientAsyncImpl.java

10. After the following change, the method is going to return early in resync case. Is it expected. Why does it not need to take the remaining operations after code change?
{code}
    if (response.getAMCommand() != null) {
      switch(response.getAMCommand()) {
      case AM_RESYNC:
          LOG.info("ApplicationMaster is out of sync with ResourceManager,"
              + " hence resyncing.");
          lastResponseID = 0;

          // Registering to allow RM to discover an active AM for this
          // application
          register();
          addOutstandingRequestOnResync();
          break
{code}
to
{code}
    } catch (ApplicationMasterNotRegisteredException e) {
      LOG.info("ApplicationMaster is out of sync with ResourceManager,"
          + " hence resync and send outstanding requests.");
      // RM may have restarted, re-register with RM.
      lastResponseID = 0;
      register();
      addOutstandingRequestOnResync();
      return null;
{code}

At last, would you mind filing a separate MR Jira, and splitting the patch into two for committing?

> Replace AM resync/shutdown command with corresponding exceptions
> ----------------------------------------------------------------
>
>                 Key: YARN-2209
>                 URL: https://issues.apache.org/jira/browse/YARN-2209
>             Project: Hadoop YARN
>          Issue Type: Improvement
>            Reporter: Jian He
>            Assignee: Jian He
>         Attachments: YARN-2209.1.patch, YARN-2209.2.patch, YARN-2209.3.patch, YARN-2209.4.patch, YARN-2209.5.patch, YARN-2209.6.patch, YARN-2209.6.patch
>
>
> YARN-1365 introduced an ApplicationMasterNotRegisteredException to indicate application to re-register on RM restart. we should do the same for AMS#allocate call also.


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