tomicooler commented on a change in pull request #3407:
URL: https://github.com/apache/hadoop/pull/3407#discussion_r705361455
##########
File path:
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
##########
@@ -1012,17 +1019,23 @@ private long getInheritedDefaultAppLifetime(CSQueue q,
// lifetime. Otherwise, use current queue's max lifetime value for its
// default lifetime.
if (defaultAppLifetimeWasSpecifiedInConfig) {
- if (parentsDefaultAppLifetime <= myMaxAppLifetime) {
- defaultAppLifetime = parentsDefaultAppLifetime;
- } else {
- defaultAppLifetime = myMaxAppLifetime;
- }
+ defaultAppLifetime =
+ Math.min(parentsDefaultAppLifetime, myMaxAppLifetime);
} else {
// Default app lifetime value was not set anywhere in this queue's
// hierarchy. Use current queue's max lifetime as its default.
defaultAppLifetime = myMaxAppLifetime;
}
} // else if >= 0, default lifetime was set at this level. Just use it.
+
+ if (myMaxAppLifetime > 0 &&
+ defaultAppLifetime > myMaxAppLifetime) {
+ throw new YarnRuntimeException(
+ "Default lifetime " + defaultAppLifetime
+ + " can't exceed maximum lifetime " + myMaxAppLifetime);
+ } else if (defaultAppLifetime <= 0) {
Review comment:
The previous if statement sets the value to
min(**parentsDefaultAppLifetime**, **myMaxAppLifetime**) or
**myMaxAppLifetime** already. Maybe there is a case when the
**parentsDefaultAppLifetime** is <= 0, that's the only reason that this was
done later:
`defaultApplicationLifetime = defaultApplicationLifetime > 0 ?
defaultApplicationLifetime : maxApplicationLifetime;`
NIT: Anyway I think this should not be an **else if**, just a standalone
**if**. That's easier to comprehend.
```
// else if...
!(myMaxAppLifetime > 0 && defaultAppLifetime > myMaxAppLifetime) &&
(defaultAppLifetime <= 0)
(myMaxAppLifetime <= 0 || defaultAppLifetime <= myMaxAppLifetime) &&
(defaultAppLifetime <= 0)
// Let's say defaultAppLifetime=0
myMaxAppLifteTime <= 0 || 0 <= myMaxAppLifetime
// So this has nothing to do with the myMaxAppLifetime
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
