Attached v0 patch for API support. Some points need to be discussed for the scenario such as

1. does update timeout should adds up for current timeout or replace it? I basically prefer to adds to the registered applications.
2. If update timeout request for unregistered application, should ignore the request or register from now?

-1 overall

This message was automatically generated.

Things like priority, timeout, node-label are attributes of application,
I'm thinking whether makes sense to have a single API which incorporates these updates. That way, we don't need to add new API method again and again.
cc Sunil G

+1 for the suggestion. This make sense to me.

Make sense to me.. But some of the options may not have any relation with each other while doing update at runtime.

For example, we may have changed only app priority while app is running. But we may end up checking app timeout and other params to dynamically update. Thoughts?

I think from server end, this can be handled without any issue like below.
if(priority!-null) doPriorityUpdate
if(ApplicationTimeouts!=null) doApplicationTimeoutsUpdate

I think we need to check from client end, and also if we plan to do this approach then these changes should go to branch-2.8 before it get released.

Yes Rohith Sharma K S.
I also thought in same line.. Then i had seen in some systems where they fill the non-changed values instead of null.

But if we can decide this approach make sense, i think its ok.

Yes..

Adding few more thoughts:

• It makes sense to have this class for dynamic/runtime updates. So if we want to change the priority/timeout of an app, then a common ApplicationAttributes class can address all such common app related attributes.
• However during submission time, its better to have these api's separately for priority/timeout. WE use existing setPriority api to achieve this, and hence its better to have these apis separately in AppSubmissionContext.
• For REST api, we have updateApplicationPriority to set priority of an app. Thinking out loud, it will be easier for clients to have separate REST api for priority or timeout. So I think we can discuss and confirm about REST side changes.

Thoughts.?

I'm thinking whether makes sense to have a single API which incorporates these updates. That way, we don't need to add new API method again and again.

I had offline discussion with Varun Vasudev for having single API which incorporates application updates. But one of the point of concern when doing multiple applications entities update, What is the return status for users when any one of the entity update fails? Say, consider that as of now priority and timeout can be clubbed into ApplicationUpdates which can be used in future. The concern is

1. update priority OR timeout only, success/failure can be identified and return corresponding error codes.
2. update priority AND timeout. What is the error code sent to User when priority is updated successfully and timeout update fails.? This scenario handling would be difficult in future also as many update entities getting added.

I think it would be better to go ahead with individual API updates only. Thoughts?

sounds good to me. we can make sure code reused at server side.

Yes. make sense for me too..

Updated working patch with tests. Brief about the patch are

1. RMAppLifetimeMonitor#monitoredApps changed its type to Map from ConcurrentMap. And explicitly handled synchronization in this class.
2. Provided an API with name updateApplicationTimeouts
3. The API updateApplicationTimeouts support only to extend the lifetime validity. It mean application can start monitoring using this API or application can extend its lifetime. But applications can't reduce its lifetime value.
4. updateApplicationTimeouts API is blocking call.

Hi Rohith Sharma K S,
Patch does not seem to get applied on trunk please check and also it would be helpful to see the modified approach captured in the document for understanding newer approach.

Thanks Rohith Sharma K S, few high level comments.
1.

	    long newLifetime = newAppTimeouts.getLifetime();
            if (newLifetime < 0) {

newLifetime can be 0. Is there any special meaning for timeout value of 0. i could not find any special handling for that.
If there are no special handling, we could validate that out to avoid some extra code cycles.

2. rmContext.getRMAppLifetimeMonitor().updateApplicationLifetime() needs to check whether app was already registered earlier.
3. I think we can publish this information to timeline also through appUpdated event .

• I thought the lifetime passed in the updateLifetime API is an absolute value, looks like it's extra ? should it be absolute ? so that shorten it is also possible in future.
• For below code , appTimeouts.setLifetime will throw NPE if appTimeout is null.

      ApplicationTimeouts appTimeouts =
          application.getApplicationSubmissionContext().getApplicationTimeouts();
      if (appTimeouts != null) {
        oldLifetime =
            appTimeouts.getLifetime() > 0 ? appTimeouts.getLifetime() : 0;
      }
      appTimeouts.setLifetime(oldLifetime + newLifetime);
  

• The updateApplicationTimeout will store the updated applicationSubmissionContext, it is possible that this happens even before the original appSubmissionContext is persisted when app is submitted. Then the updated timeout will be overwritten.

• similarly for below code, the newLifetime could be overwritten by the original timeout in appSubmissionContext on submission. then we lost the updated timeout.

        // If application is not monitored earlier then start monitoring from now
        register(appId);
        monitoredApps.put(appId, newLifetime);
  

  so should we allow update only when app is running or accepted?

Attached the patch. Brief about the patch are

1. ApplicationClientProtocol is added with new method i.e updateApplicationTimeouts method.
2. UpdateApplicationTimeoutsRequest takes appId and Map<ApplicationTimeoutType, Long> to update timeout values. And these timeout values are gets added to existing timeouts out. If application was not monitored, then this application start monitoring from now.
3. New API updateApplicationTimeouts added in RMAppLifetimeMonitor class. This is basically updates timeout for given ApplicationTimeoutTypes.
4. New field appIdToTimeoutTypeMapping is added in RMAppLifetimeMonitor. This is to track each appIdToApplicationTimeoutTypes mapping where can be used as cache.

-1 overall

This message was automatically generated.

• The first if condition already checks whether the app is either at new or new_saving, then the second if COMPLETED_APP_STATES condition will never be true ?

      if (EnumSet.of(RMAppState.NEW, RMAppState.NEW_SAVING)
          .contains(application.getState())) {
        if (COMPLETED_APP_STATES.contains(application.getState())) {
          RMAuditLogger.logSuccess(callerUGI.getShortUserName(),
              AuditConstants.UPDATE_APP_TIMEOUTS, "ClientRMService",
              applicationId);
          return response;
        }
  

• The appIdToTimeoutTypeMapping may be not needed. You can construct a new RMAppToMonitor instance with the provided appId and the timeoutType and check whether the instance exists in 'monitoredApps' or not. If it exists, update the corresponding timeout value. If it doesn't exist, instert the RMAppToMonitor instance into the map.

      for (Entry<ApplicationTimeoutType, Long> entry : timeouts.entrySet()) {
        ApplicationTimeoutType timeoutType = entry.getKey();
        RMAppToMonitor rmAppToMonitor = timeoutTypeMapping.get(timeoutType);
        long newTimeout = entry.getValue();
        if (rmAppToMonitor == null) {
          // If application is not registered earlier, register and start
          // monitoring from now
          RMAppToMonitor appToMonitor = new RMAppToMonitor(appId, timeoutType);
          register(appToMonitor);
          monitoredApps.put(appToMonitor, newTimeout * MS);
          timeoutTypeMapping.put(timeoutType, appToMonitor);
        } else {
          // Already registered app, just update time out.
          Long appTimeout = monitoredApps.get(rmAppToMonitor);
          newTimeout = newTimeout + (appTimeout / MS);
          monitoredApps.put(rmAppToMonitor, newTimeout * MS);
        }
        newTimeouts.put(timeoutType, newTimeout);
      }
  

• Also, when we update the timeout, the new timeout should be current timestamp + newTimeout value. Later, we will also send the remaining lifetime to user if user queries, this way, it's easier to reason - what user sets as the timeout value is what user will get when he queries.

The appIdToTimeoutTypeMapping may be not needed.

I added this for couple of reasons.

1. Caching, need not to create RMAppToMonitor object on every update or on getRemainingTime or any other.
2. Since, YARN may support more timeout for an application, it would help to track what all are the timeouts for an application has been registered. Say app1 might want only lifetime, app2 might want lifetime & queue_time. In such cases, it is easier to look up this mapping and get registered timeouts for an application.

Also, when we update the timeout, the new timeout should be current timestamp + newTimeout value. Later, we will also send the remaining lifetime to user if user queries, this way, it's easier to reason - what user sets as the timeout value is what user will get when he queries.

Good point. let me change it. Note: as of now, we are not storing timeout values in statestore apart from submissionContext. Submission context will contains only timeout which is not absolute. But, currently we are supporting only lifetime, then we can recover on RM restart. So, in future if any such use case for supporting different timeouts then for RM HA cases, we need to recover either monitoringStartTime or EndTime.

Updated patch addressing comments.

-1 overall

This message was automatically generated.

Thanks Rohith,

• AbstractLivenessMonitor#getMonitorStartTime not used
• I think we could change AbstractLivelinessMonitor to remember the finalExpireTime instead of remembering the monitorStartTime and the expiryInterval.
  This way, RMAppLifetimeMonitor does not need to register an app with zero startTime. And the monitoredApps map may be not needed, the running map in AbstractLivelinesMonitor may be enough

Summary of discussion for supporting update API for timeout with Vinod Kumar Vavilapalli, Varun Vasudev and Sunil G.

Current Patch Approach: update API is synchronous call. And its timeout parameter affect from now i.e targetTimeoutFromNow(in seconds) for given ApplicationTimeoutTypes. Say, user wants to update timeout, if user passes 30 seconds for update then RM will calculate (now + 30 seconds ) and store in state store.

Concern on update API approach

1. synchronous call is too expensive. In case of statestore problem, other RPC calls are blocked.
   So basically approach of synchronous call need to be changed to asynchronous call. Note, updatePriority is also blocking call which need to be solved once server side supported.

Challenges

1. Yarn-client need to be adopt polling mechanism for RM for its update confirmation. The first important thing need consider for polling mechanism is Idempotent API. So, to the user, input remain relative, but client need to convert relative time to targetedTime and send a request to RM with targeted-time. The RM takes requested targeted timeout as-is and use it. Say, user input timeout as 30 seconds, then yarn client change it to targetedTimeout=(now + 30seconds) and sends to RM. To the RM, update request is always targeted time.
2. Multiple client update timeout for same application. Here, issue is how does yarn-client get to know about its update confirmation.? How long yarn-client need to re-try/wait before bailing out.?
3. More importantly, Timezone issue if yarnclient converts it as explained in first challenge. Say, RM is running in IST , and yarnclient is running in PDT timezone. Even though Yarnclient converts user input to targeted time and send to RM, since both client-server timezone are different, the targetedTimeout value is totally different.

Some of the thoughts solving the challenges

1. Server(RM)
   1. Always accept targeted timeout in UTC format and store in state-store as UTC format only. For RM monitoring, covert UTC timestamp to local machine timezone and use it. This solves Timezone issue from RM.
   2. Once update request received by RM, firstly RM should store new value in statestore. Secondly, update to monitoring service.
   3. RM should check targeted timeout is same as request time.If yes, return a boolean flag indicating update is success. Always keep a copy of updated timeout values in RMApp and return these values when somebody query for timeout values.
2. YarnClient:
   1. Convert user input timeout to targeted-timeout in UTC and send update request to RM
   2. Keep sending update request in loop as long as response received as true indicating update confirmation.
   3. Bail out and fail the client operation after some configured value.

Any comments on approach are welcome.

As you noted, making it polling is introducing all these complexities. IMO, we can rely on HADOOP-11552 which allows RPC handler to hand off the work to other threads. So we won't run out of handler threads. This way, it's much simpler.

HADOOP-11552 may be an easy out as long as we agree it makes into the next release where app-timeouts feature and update-priorities exists. Otherwise, we risk regressing on high-availability. Let's make sure this is a blocker.

OTOH, even assuming HADOOP-11552, there is one API concern I have. If multiple users start trying to update the timeout for a single application, the behavior from individual user's point of view is arbitrary. Both the user's clients block, and after the call returns, the timeout may be set to the value that they asked for or what the other user asked for.

To avoid this, the API should be something like targetTimeoutFromNow(long abSoluteTimeoutValueAtRequest, targetTimeout) so that the server can reject requests if the current recorded value changes by the time the request is accepted. Essentially this is Read -> CompareAndSetOrFail.

We (me Jian He and Vinod Kumar Vavilapalli) had offline discussion regarding this. Here are the consensus taken.
RM Server :

1. RM accept always targetedExpireTime in UTC.
2. RM should take care of transactional update to state store and time conversion to local time zone to monitor application.

Protocol changes.

1. update API(YarnClient->RM ) should be an Idempotent.
2. update Response from RM should be currentExpiryTime and updatedExpiryTime. Here, user can validate from and to time.

YarnClient :

1. Takes the relative input from user i.e from-Now.
2. yarn client convert user relative value to targetedExpireTime(=Now + relative value) into UTC format and send a request to RM.
3. In update response from and to value has been sent by RM for user reference.

To the user,

1. Input is always relative from-Now. Let say, current time is 10 AM, and application expiry time is 11 AM. If user wants to update expire time to 10.30 AM then User need to input 30 minutes.

@Jian and @Vinod, please add more if I have missed any points.

Thanks folks for the summary and approach looks fine. Couple of quick questions:

1. update Response from RM should be currentExpiryTime and updatedExpiryTime. Here, user can validate from and to time.
>> If currentExpiryTime and updatedExpiryTime is less than current time from RM, I suppose we will fail this request. Correct?

2. In update response from and to value has been sent by RM for user reference.
>> From targetedExpireTime(=Now + relative value), I think api will return from as Now or Current Time and to as targetedExpireTime. UTC conversion was done in YarnClient/RM to keep a standard, but client is unaware of same. I think this timestamps could be given in client's timezone. It may be more easier.

update Response from RM should be currentExpiryTime and updatedExpiryTime. Here, user can validate from and to time.

Will currentExpiryTime and updatedExpiryTime be in UTC?

yarn client convert user relative value to targetedExpireTime(=Now + relative value) into UTC format and send a request to RM.

Related to my point above, if the ApplicationReport and updateResponse are reporting back expiry time in UTC, then we should provide an API that lets you set the expiry time in UTC(in addition to the API that lets you set expiry time relative to now)

Will currentExpiryTime and updatedExpiryTime be in UTC?

Yes, YarnClient<-->RM communication is only with UTC format. There will be utility methods to convertToUTC(localTime) and convertToLocalTimezone(timeInUTC) provided that is used by yarnClient as well as RM. Once update Response received by YarnClient , yarnClient convert to local time-zone and give it back to user.

Instead of using long as part of the API and expecting clients to convert time to and from UTC epoch, it's much cleaner to use a ISO-8601 formatted string. You can avoid writing the utility functions as well since there are a lot of libraries to handle ISO-8601 dates.

Attaching the patch for an update API server side implementation. Though patch looks bit heavy, most of the changes are in test. Apart from test modifications, the actual modification is as below.

1. There is protos added in ApplicationClientProtocol for updateApplicationTimeout. This adds 4 files corresponding request/response with PBImpl.
2. Similarly, there are protos added AppUpdateAttributes to store in StateStore. This adds 4 new files with PBImpl.

The main module interaction point is ClientRMService->RMAppImpl->RMStateStore. The interaction flow is as below.
ClientRMService : Accept incoming request from client and validate requested data. Trigger an event to RMAppImpl to update app timeout. And wait for update success response. In case of state store failure, roll back updated attributes.
RMAppImpl : It handles new event APP_UPDATE and update in monitoring service. And trigger an event to update in State Store in order.
RMStateStore : Store updated app state and set the future object which is passed by clientRMService. Once RMStateStore sets future object, clientRMService return to user as success.

Pending task : Currently update response is returning empty. But response should contains updated value. I will update this behavior in next patches.

-1 overall

This message was automatically generated.

• What is the reason to involve RMAppImpl state-machine to update the application timeout ? I think it's simpler to update directly, not go through the RMAppImpl state machine..
• This code

      Map<ApplicationTimeoutType, Long> newApplicationExpireTime =
          new HashMap<ApplicationTimeoutType, Long>();
      newApplicationExpireTime.putAll(newParsedTimeouts);
  

  can be changed to

      Map<ApplicationTimeoutType, Long> newApplicationExpireTime =
          new HashMap<ApplicationTimeoutType, Long>(newParsedTimeouts);
  

• I wonder whether we need a separate wrapper class "AppUpdateAttributes" for the timeout, is it fine to just put application timeout in the ApplicationStateData class?

Updated patch with following changes from previous patch

1. Removed RMAppImpl state transition and made direct call to RMAppImpl.
2. Made updateTimeout API as transactional.
3. Fixed test cases failures.
4. Fixed review comment i.e removed application attribute class and storing in ApplicationStateData only.

Pending task

1. UpdateResponse is empty in current patch. As we discussed we need to send back response as updated timeout value. This I will add in next patches.
2. Checkstyle issue will be handled.

-1 overall

This message was automatically generated.

• can you add comments about the type of the timeout value ?

    /**
     * Set the <code>ApplicationTimeouts</code> for the application in seconds.
     * All pre-existing Map entries are cleared before adding the new Map.
     * 
  

• Revert RMAppEventType change
• for roll back, there's no need to use the future object.

          // do roll back
          future = SettableFuture.create();
          app.updateApplicationTimeout(RMAppUpdateType.ROLLBACK, newExpireTime,
              currentApplicationTimeouts, future);
          // Roll back can fail only when application is in completing state.
          try {
            Futures.get(future, YarnException.class);
          } catch (YarnException e) {
            LOG.warn("Roll back failed for an application "
                + app.getApplicationId() + " with message" + e.getMessage());
          }
  

• fix indentation of second line

        for (Map.Entry<ApplicationTimeoutType, Long> timeout : 
        app.applicationTimeouts.entrySet()) {
  

Thanks Rohith Sharma K S for the patch.

Few comments

1. ApplicationUpdateTimeoutMapProto -> ApplicationUpdateTimeoutProto. May not need to have "map" in the name of proto.
2. Few more comment about the map "applicationTimeouts" in updateApplicationTimeouts of ClientRMService
3. I think we need to create an event internally here for RPCUtil.getRemoteException.

    if (applicationTimeouts.isEmpty()) {
      String message =
	       "At least one ApplicationTimeoutType should be configured for updating timeouts.";
      RMAuditLogger.logFailure(callerUGI.getShortUserName(),
	          AuditConstants.UPDATE_APP_TIMEOUTS, "UNKNOWN", "ClientRMService",
	          message, applicationId);
      throw RPCUtil.getRemoteException(message);
   }

4. There is a any chance that NEW_SAVING could fail and app could no longer be in next state. We could consider apps which are from ACCEPTED state, correct? Any specific reason to consider NEW_SAVING?

	    if (RMAppState.NEW.equals(application.getState())
	        || COMPLETED_APP_STATES.contains(application.getState())) {

5. In validateISO8601AndConvertToLocalTimeEpoch, instead of using long currentTimeMillis = System.currentTimeMillis();, its better to use Clock.
6. In validateISO8601AndConvertToLocalTimeEpoch, if (expireTime < currentTimeMillis) helps to validate timeout's of past. But still there could be a delta time from current time to next timeout check thread. Could that also to be validated here? Or such entries will be discarded by the monitor thread?
7. In general thought and thinking out loud, Map<ApplicationTimeoutType, Long> }} why we need a map here. Could we have {{List<ApplicationTimeouts> and each ApplicationTimeouts could have its type and real timeout value in long. I am not seeing much of benefit from map here as app timeout types could no be more than 3/4 atmost.

1. ApplicationUpdateTimeoutMapProto -> ApplicationUpdateTimeoutProto. May not need to have "map" in the name of proto

It is naming convention I maintained with other map protos. Do not want to change its naming convention.

There is a any chance that NEW_SAVING could fail and app could no longer be in next state. We could consider apps which are from ACCEPTED state, correct? Any specific reason to consider NEW_SAVING?

Update timeout can also possible in NEW_SAVING state. This is because state-store update is done from dispatcher queue which ensures that updateTimeout event will go always behind new_saving event. So, should not be problem updating in new_saving. In case, new_saving fails, then App it self is failed.

In validateISO8601AndConvertToLocalTimeEpoch, if (expireTime < currentTimeMillis) helps to validate timeout's of past. But still there could be a delta time from current time to next timeout check thread. Could that also to be validated here? Or such entries will be discarded by the monitor thread?

When dealing with milliseconds, there will be delta changes. It should be fine. Actually, as user I would expect to update timeout at least with 5minutes gap.

In general thought and thinking out loud, Map<ApplicationTimeoutType, Long> }} why we need a map here. Could we have {{List<ApplicationTimeouts> and each ApplicationTimeouts could have its type and real timeout value in long. I am not seeing much of benefit from map here as app timeout types could no be more than 3/4 atmost.

Good question. There are couple of reason for considering MAP instead of List.

1. It is defensive code style, where RM do not want to expect any duplicate applicationTimeout types. If we go for List, user is allowed to input duplicate timeout types so that RM server need to take decision which one to consider and it is debatable topic. Advantage of is MAP is user send single timeout types. It is more clarity for user as well as RM.
2. Populating MAP is much easier in RM. In List, need to look up for all the entry correspond to timeoutType. Let say, in new_saving, timeout update is for LIFETIME. It is easier to look up and get value in map. But in list, on every state need to iterate for all the values and find does LIFETIME exist or not.

updated patch fixing review comments.. I have updated java doc and updateRequestResponse in this patch.

-1 overall

This message was automatically generated.

Currently, the patch is taking a lock on the applicationId when updating timeout, if we do update in store first and then update in memory, we will not need the rollback related code ?  

Updated patch fixing allowing priority and timeout update parallel.

-1 overall

This message was automatically generated.

Thanks Rohith Sharma K S for the patch..

Few more comments.
1. ApplicationClientProtocol#updateApplicationTimeouts .Could this be an Evolving api?.
2. ApplicationClientProtocolPBClientImpl#updateApplicationTimeouts. Does exception handling block needs return? RPCUtil method will throw exception, correct?
3. In ApplicationClientProtocolPBServiceImpl#updateApplicationTimeouts, we use catch (YarnException| IOException e).
4. On a different note, i think COMPLETED_APP_STATES could be defined by RMAppImpl itself and expose a read-only api. This can help to cleanup local states definitions. could be done in another patch.
5. Given a writeLock in RMAppImpl#updateApplicationTimeout, why do we need another lock in RMAppManager#updateApplicationTimeout. Is this to handle some race conditions while app update event is waiting in StateStore dispatcher queue? I would love to have some more comments in these synchronized blocks or write locks to give a brief explanation. It will help us later.
6. RMApp is generally considered as read-only. updateApplicationTimeout will violate that. we can place this api in RMAppImpl itself, and in client side, we could convert to RMAppImpl object and use. ProportionPolicy, New Global Scheduler etc are using this way.
7. Timeout is to be part of ApplicationReport correct? Is that a part of this patch?

-1 overall

This message was automatically generated.

Thank you Rohith Sharma K S for patch

Few Minor comments

1. YarnConfiguration.DEFAULT_RM_APPLICATION_LIFETIME_MONITOR_INTERVAL_MS variable name can also be updated.
2. Since multiple application timeout we might support in future can move to separate AppTimeoutDignostics

   79	    String diagnostics =
   80	        "Application killed due to exceeding its lifetime period";
   

3. Timeline server update during update of timeout also we might have to handle

   UpdateApplicationTimeoutsRequest takes appId and Map<ApplicationTimeoutType, Long> to update timeout values. And these timeout values are gets added to existing timeouts out. If application was not monitored, then this application start monitoring from now.

4. Application timeout is set to 0 and then start monitoring is it possbile can we add a testcase

Sunil G
bq 1. ApplicationClientProtocol#updateApplicationTimeouts .Could this be an Evolving api?.
the API is already UnStable, I think should be fine. cc:/Jian He

2. ApplicationClientProtocolPBClientImpl#updateApplicationTimeouts. Does exception handling block needs return? RPCUtil method will throw exception, correct?

3. In ApplicationClientProtocolPBServiceImpl#updateApplicationTimeouts, we use catch (YarnException| IOException e).

frankly I just copy pasted from other API implementation from PBImpl. It looks to be all API are doing the same way. If really want to clean, we can take separate task to do refactoring PBClientImpl classes. Thoughts?

5. Given a writeLock in RMAppImpl#updateApplicationTimeout, why do we need another lock in RMAppManager#updateApplicationTimeout. Is this to handle some race conditions while app update event is waiting in StateStore dispatcher queue? I would love to have some more comments in these synchronized blocks or write locks to give a brief explanation. It will help us later

Good question!! Jian also had same doubt. Let me brief aobut it. Anything holding writeLock in RMAppImpl blocks the main AsyncDiapatcher. This is costly operation. For updateTimeout, we need to wait for transaction to complete which can not be done holding RMAppImpl lock. So, in-memory updations happens from RMAppImpl and trigger an event to StateStore and release the writeLock. Wait for statestore update call from RMAppManager in separate lock.

4. On a different note, i think COMPLETED_APP_STATES could be defined by RMAppImpl itself and expose a read-only api. This can help to cleanup local states definitions. could be done in another patch.

I would prefer to do in separate JIRA. May be can you raise it?

6. RMApp is generally considered as read-only. updateApplicationTimeout will violate that. we can place this api in RMAppImpl itself, and in client side, we could convert to RMAppImpl object and use. ProportionPolicy, New Global Scheduler etc are using this way.

I would prefer to add in RMApp itself rather than type casting in RMAppManager. It is internal interface used for both read and write.

7. Timeout is to be part of ApplicationReport correct? Is that a part of this patch?

this will be done in YARN-4206

YarnConfiguration.DEFAULT_RM_APPLICATION_LIFETIME_MONITOR_INTERVAL_MS variable name can also be updated.

make sense to me.

Timeline server update during update of timeout also we might have to handle

Timeout is internal to rmapp. I think adding in ATS is NOT required. I think can we move this discussion to separate thread?

bq Application timeout is set to 0 and then start monitoring is it possbile can we add a testcase
currently, if timeout is 0, update API will fail.

Thanks Rohith Sharma K S. I think RMContainer is mostly kept as read-only. I have not see any comments or history specifically for RMApp. You could help to confirm the same.

updating patch with following changes with typical transaction approach.

1. Store it in RMStateStore.
2. Update in-memory.
3. No two updates for same application can go together.

Hi Rohith Sharma K S,
Few more comments:

1. StateStore exception is thrown back as it is. Its better to we can wrap and send back error to client with more informations such as appId etc.

2. As per a comment from Jason earlier in MAPREDUCE-5870, we need not have to throw exception for apps which are completing. A similar approach is done for app priority in YARN-4141. I think we might need to consider same for app timeouts too.

-1 overall

This message was automatically generated.

Updated patch fixing review comments and findbug issue.

Thanks Rohith Sharma K S
Patch looks generally fine for me pending jenkins.

-1 overall

This message was automatically generated.

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10806 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10806/)
YARN-5611. Provide an API to update lifetime of an application. (jianhe: rev bcc15c6290b3912a054323695a6a931b0de163bd)

• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/RMApp.java
• (add) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/protocolrecords/UpdateApplicationTimeoutsRequest.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/proto/yarn_server_resourcemanager_recovery.proto
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/ClientRMService.java
• (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/TestClientRedirect.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/AbstractLivelinessMonitor.java
• (add) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/api/protocolrecords/impl/pb/UpdateApplicationTimeoutsResponsePBImpl.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMServerUtils.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/ApplicationClientProtocol.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/Times.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAuditLogger.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/monitor/RMAppLifetimeMonitor.java
• (add) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/api/protocolrecords/impl/pb/UpdateApplicationTimeoutsRequestPBImpl.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/MockRMApp.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/recovery/records/ApplicationStateData.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/resources/yarn-default.xml
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/recovery/RMStateUpdateAppEvent.java
• (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-api/src/main/proto/yarn_service_protos.proto
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/MockResourceManagerFacade.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/api/impl/pb/client/ApplicationClientProtocolPBClientImpl.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-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/recovery/records/impl/pb/ApplicationStateDataPBImpl.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/proto/applicationclient_protocol.proto
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/recovery/RMStateStore.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacityScheduler.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/applicationsmanager/MockAsm.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/proto/yarn_protos.proto
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/api/impl/pb/service/ApplicationClientProtocolPBServiceImpl.java
• (add) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/protocolrecords/UpdateApplicationTimeoutsResponse.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/api/records/ApplicationSubmissionContext.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java

Committed to trunk, branch-2, thanks Rohith !
Thanks Sunil for helping the reviews !

thanks Jian and Sunil for thorough review..

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10958 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10958/)
YARN-5932. Retrospect moveApplicationToQueue in align with YARN-5611. (rohithsharmaks: rev 563480dccd0136d82730f4228f1df44449ed5822)

• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/LeafQueue.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FairScheduler.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManagerEventType.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ParentQueue.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CSQueue.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManagerEvent.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestMoveApplication.java
• (delete) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/RMAppMoveEvent.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/YarnScheduler.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/ClientRMService.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/RMAppEventType.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerNodeLabelUpdate.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/AbstractYarnScheduler.java
• (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/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java
• (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacityScheduler.java