SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10887 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10887/)
YARN-4206. Add Application timeouts in Application report and CLI. (sunil: rev eb0a483ed07399c7a06ed9db4bfbec382de470da)

• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/RMAppImpl.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/TestApplicationLifetimeMonitor.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/main/java/org/apache/hadoop/yarn/client/api/YarnClient.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/cli/TestYarnCLI.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/ApplicationReport.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/main/java/org/apache/hadoop/yarn/client/api/impl/YarnClientImpl.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/main/java/org/apache/hadoop/yarn/client/cli/ApplicationCLI.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/api/records/impl/pb/ApplicationReportPBImpl.java
• (add) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/ApplicationTimeout.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/proto/yarn_protos.proto
• (add) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/api/records/impl/pb/ApplicationTimeoutPBImpl.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/api/TestPBImplRecords.java

Thanks Rohith Sharma K S for the patch. Thanks Jian He Vinod Kumar Vavilapalli Gour Saha for additional reviews!

YARN-5920 would have fixed by TestRMHA failure. However it seems this jenkins run was not having that fix.

+1 to latest patch. Committing shortly..

-1 overall

This message was automatically generated.

+1

After having discussion with Jian and other folks, they feel that API should be more clarity and easy enough to input values. Providing multiple options makes user to bit confusing state. So, going ahed with 2nd suggestion i.e ./yarn application -appId $appId -updateLifetime $timeout.
Attached the patch reflect the same.

would a single option -updateLifetime be simper ? having one more option makes user memorize one more option...

+1 overall

This message was automatically generated.

Updated patch fixing Vinod review comments.

Few comments:

• Rename suggestions
  • getApplicationTimeoutList -> getApplicationTimeouts & setApplicationTimeoutList -> setApplicationTimeouts
  • getAppTimeoutType -> getTimeoutType & setAppTimeoutType -> setTimeoutType
  • getExpireTime -> getExpiryTime
• Drop the "of an application" part in the javadoc everywhere to keep it open for app, app-attempt, container timeouts? For e.g. in "Get <code>expireTime</code> of an application for given timeout type."
• From the server side, we should always return a timeout object - for now only with LIFETime type.
  • If an app didn't specify anything: expiry-time = "unlimited", remaining-time = -1 (unknown)
  • If an app specified the timeout and is running, return proper values
  • If an app is finished, expiry-time = <proper value | unlimited>, remaining-time = 0
• CLI
  • I think we should either implement (a) your earlier proposal (./yarn application -updateTimeout 10 -updateType LIFETIME) as it is foolproof., then madate updateType with updateTimeout and for now only support LIFETIME as a type in the CLI or (b) have a simpler updateLifetime.

  • +        ApplicationTimeout timeout =
    +            appReport.getApplicationTimeoutList().iterator().next();
    +        appReportStr.print("\tApplication Expire Time : ");
    +        appReportStr.println(timeout.getExpireTime());
    +        appReportStr.print("\tApplication Remaining Time : ");
    +        appReportStr.print(timeout.getRemainingTime());
    

  • We should simply iterate the timeouts and print one line for each type: "Lifetime: expiry-time=<ISO format>, remaining-time=<long>seconds"

-1 overall

This message was automatically generated.

updated patch fixing test failure.

It is related to patch, let me upload a new patch with test case correction.

UT failed, Rohith Sharma K S, could you take a look ?

-1 overall

This message was automatically generated.

+1

updated patch with following changes

1. fixed Jian review comment i.e log message change
2. Removed support for sub option i.e -updateType since only one timeout type is supported. In future if any plan to support new timeout, then LIFETIME will become default value. New CLI API remain as ./yarn application -appId <appId> -updateTimeout <Timeout>

minor comment on the logging

 sysout.println("Updating timeout of an application " + applicationId);
sysout.println("Successfully updated timeout of an application "
        + applicationId + ". New expire time will be " + newTimeout);

may be explicitly mention the timeoutType ? like:

 sysout.println("Updating " + timeoutType + " of an application " + applicationId);
sysout.println("Successfully updated " + timeoutType +" of an application "
        + applicationId + ". New expire time will be " + newTimeout);

-1 overall

This message was automatically generated.

Updating patch fixing Jian Review comments and few checkstyle and test case failures.

Currently, updated patch do not consider default value and expect user to input timeout type mandatary. We might feel sub option -updateType is not required as of now since only single timeout considered. But it is better to keep it now considering user facing do not change later if more timeouts supported. May be we can define default value when more timeouts are supported?

Rohith Sharma K S
I think we can take the default behavior to LIFETIME even if there are no other sub-options with updateType.

updateTimeout, maybe call it updateLifeTimeout?

How about passing flag, this can be resued in future without changing CLI interface.
Ex : ./yarn application -updateTimeout 10 -updateType LIFETIME

looks good overall ,thanks Rohith, few comments

• what is this code for, we can remove?

        if (this.applicationTimeouts.isEmpty()) {
  
        } else {
  
        }
  

• could you add comments for the API in ApplicationTimeout ?
• updateTimeout, maybe call it updateLifeTimeout?

-1 overall

This message was automatically generated.

Updating patch for supporting YarnClient API, ApplicationCLI to print timeout in status command , support for new option -updateTimeout. And modification is in ApplicationReport to get list of timeouts values.

In this JIRA, lets handle ApplicationReport changes and CLI part. For the yarn web UI, it is required to change for both new and old UI, so lets handle it YARN-5650 separately. I will update JIRA summary to reflect the same.

ahh.. In LIFETIME monitoring, application attempt might get pre-empted, but lifetime timeout is considered from app submitted to app finished. So, in between any number of application-attempt can launch and preempt, its not an issue because app state is still running. Above mentioned(your comment) scenario need to think of for other types of timeout that we are planning to support.

When the app is back into RUNNING state at 10:10:35 what will the value of remaining timeout and absolute timeout be?

Thats a good question For the applications which are completed or applications which are not enabled with timeout, report should have special value i.e -1 which indicates either application is completed or timeout not enabled. In your case, timeout value will be set to -1 which indicates application is in completing states.

What happens when the app gets pre-empted at 10:10:15 for 20 secs?

Isn't absolute timeout going to be inaccurate?

No, It will be fixed time. Lets say, I have submitted application with 10seconds timeout. Assume job submitted time is 10:10:10 PM. So absolute time for application to get timed out is 10:10:20. After 3 seconds, if user request for app-report, then both 10:10:20 PM as absolute time and 7 secs as remaining time could be sent. Absolute time never changes, but remaining time changes on each user request.

Isn't absolute timeout going to be inaccurate? Given that we know the remaining timeout of an application, what we don't know is that going forward will it be pre-empted or not. If it is, then the absolute value will move further down? Is my understanding incorrect?

I don't think we need an additional API in ApplicationClientProtocol to get that.

Thats true. We do not really require new client API.

Additionally If I look at the user use cases, user would require both absolute time and remaining time. Say if timeout is less than a day or half a day, then remaining timeout would be sufficient. If timeout is 3-4 days later, user might interested in knowing which day it will get time out. So, How about new object ApplicationTimeout which gives both absolute-time and remaining-time for given ApplicationTimeoutType? The ApplicationReport would consists of Map<ApplicationTimeoutTypes, ApplicationTimeout>. Thoughts?

The remaining time can be inferred from the absolute timeout value ? If so, I don't think we need an additional API in ApplicationClientProtocol to get that.

Thanks Gour Saha for bringing out point remaining time for an application. I think application-report should contains absoluteTimeout value i.e when it gets expired. To the client, there could be new API ApplicationClientProtocol#getApplicationRemainingTime(ApplicationId) which gets remaining timeout in seconds. Thoughts? Sunil G Jian He Vinod Kumar Vavilapalli

It will be very helpful from an end-user perspective to provide the remaining-timeouts of the application in the report. For example, remaining-lifetime of an application will let the app-owner understand that he/she needs to extend the lifetime because the initial estimate of completing a task was not accurate. Note, timeouts update is going to supported as per YARN-5611.

It might not be easy or straightforward to calculate the remaining-timeouts for all ApplicationTimeoutTypes (we now support multiple timeouts). However we should try to provide them for whichever ones we can. The reason I am adding this note is because, we should not discard the idea of providing the remaining-timeouts, just because remaining-timeout for one of the ApplicationTimeoutType cannot be calculated or does not makes sense.