-
Notifications
You must be signed in to change notification settings - Fork 9.2k
YARN-10949. Simplify AbstractCSQueue#updateMaxAppRelatedField and fin… #3500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
f3bc59c
e5d0619
affcba2
cb26800
4b46cc7
5220863
3edeac3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -20,7 +20,6 @@ | |||
|
|
||||
| import java.io.IOException; | ||||
| import java.util.*; | ||||
| import java.util.Map.Entry; | ||||
| import java.util.concurrent.ConcurrentHashMap; | ||||
| import java.util.concurrent.ConcurrentMap; | ||||
|
|
||||
|
|
@@ -1939,8 +1938,9 @@ public void updateClusterResource(Resource clusterResource, | |||
| updateAbsoluteCapacities(); | ||||
|
|
||||
| super.updateEffectiveResources(clusterResource); | ||||
| super.updateMaxAppRelatedField(csContext.getConfiguration(), | ||||
| this); | ||||
|
|
||||
| // Update maximum applications for the queue and for users | ||||
| updateMaximumApplications(csContext.getConfiguration()); | ||||
|
|
||||
| updateCurrentResourceLimits(currentResourceLimits, clusterResource); | ||||
|
|
||||
|
|
@@ -2326,6 +2326,57 @@ public void stopQueue() { | |||
| } | ||||
| } | ||||
|
|
||||
| void updateMaximumApplications(CapacitySchedulerConfiguration conf) { | ||||
| int maxAppsForQueue = conf.getMaximumApplicationsPerQueue(getQueuePath()); | ||||
|
|
||||
| int maxGlobalApplications = conf.getGlobalMaximumApplicationsPerQueue(); | ||||
| int maxSystemApplications = conf.getMaximumSystemApplications(); | ||||
| int baseMaxApplications = maxGlobalApplications > 0 ? | ||||
| maxGlobalApplications : maxSystemApplications; | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think, this should be more like a Math.min situation, if the maxGlobal is defined, it is a per queue limit, while max System is the total number of applications, so if System is set, it should be always a limiting factor. Also if System is too high, then maxGlobal should be the limiting factor, so it's more like maxGlobalApplications > 0 ? Math.min(maxGlobalApplications, maxSystemApplications) : maxSystemApplications;
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree on this and this change still conforms to our existing tests.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem with this approach is that system max app defaults to 10000. This means you basically can not set maxGlobalApp higher than this value, because it will be trimmed to 10000. We could pursue this path by making maxSystemApp defaults to -1 and check if it is defined, but I am not convinced it is worth the effort. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the documentation: Line 1044 in 5337beb
So capacity scheduler will enforce the max system application anyway, even if the leaf queue would allow it, so there is no point in allowing more application per leaf queue than the system wide max applications. |
||||
|
|
||||
| String maxLabel = RMNodeLabelsManager.NO_LABEL; | ||||
| if (maxAppsForQueue < 0) { | ||||
| if (maxGlobalApplications > 0 && this.capacityConfigType | ||||
| != CapacityConfigType.ABSOLUTE_RESOURCE) { | ||||
| maxAppsForQueue = maxGlobalApplications; | ||||
| } else { | ||||
| for (String label : configuredNodeLabels) { | ||||
| int maxApplicationsByLabel = (int) (baseMaxApplications | ||||
| * queueCapacities.getAbsoluteCapacity(label)); | ||||
| if (maxApplicationsByLabel > maxAppsForQueue) { | ||||
| maxAppsForQueue = maxApplicationsByLabel; | ||||
| maxLabel = label; | ||||
| } | ||||
| } | ||||
| } | ||||
| } | ||||
|
|
||||
| setMaxApplications(maxAppsForQueue); | ||||
|
|
||||
| updateMaxAppsPerUser(); | ||||
|
|
||||
| LOG.info("LeafQueue:" + getQueuePath() + | ||||
| "update max app related, maxApplications=" | ||||
| + maxAppsForQueue + ", maxApplicationsPerUser=" | ||||
| + maxApplicationsPerUser + ", Abs Cap:" + queueCapacities | ||||
| .getAbsoluteCapacity(maxLabel) + ", Cap: " + queueCapacities | ||||
| .getCapacity(maxLabel) + ", MaxCap : " + queueCapacities | ||||
| .getMaximumCapacity(maxLabel)); | ||||
| } | ||||
|
|
||||
| private void updateMaxAppsPerUser() { | ||||
| int maxAppsPerUser = maxApplications; | ||||
| if (getUsersManager().getUserLimitFactor() != -1) { | ||||
| int maxApplicationsWithUserLimits = (int) (maxApplications | ||||
| * (getUsersManager().getUserLimit() / 100.0f) | ||||
| * getUsersManager().getUserLimitFactor()); | ||||
| maxAppsPerUser = Math.min(maxApplications, | ||||
| maxApplicationsWithUserLimits); | ||||
| } | ||||
|
|
||||
| setMaxApplicationsPerUser(maxAppsPerUser); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Get all valid users in this queue. | ||||
| * @return user list | ||||
|
|
||||
Uh oh!
There was an error while loading. Please reload this page.