YARN-10949. Simplify AbstractCSQueue#updateMaxAppRelatedField and fin… #3500
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d a more meaningful name for this method
|
💔 -1 overall
This message was automatically generated. |
tomicooler
suggested changes
Sep 30, 2021
...n/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
tomicooler
reviewed
Oct 5, 2021
...rc/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/LeafQueue.java
Outdated
Show resolved
Hide resolved
tomicooler
approved these changes
Oct 5, 2021
shuzirra
requested changes
Oct 7, 2021
| int maxGlobalApplications = conf.getGlobalMaximumApplicationsPerQueue(); | ||
| int maxSystemApplications = conf.getMaximumSystemApplications(); | ||
| int baseMaxApplications = maxGlobalApplications > 0 ? | ||
| maxGlobalApplications : maxSystemApplications; |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
I agree on this and this change still conforms to our existing tests.
There was a problem hiding this comment.
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.
According to the documentation:
Maximum number of applications in the system which can be concurrently active both running and pending. Limits on each queue are directly proportional to their queue capacities and user limits. This is a hard limit and any applications submitted when this limit is reached will be rejected. Default is 10000
This means to me, that maxSystemApp should be an absolute upper limit, if the user wishes to go above it, they should raise this limit. MaxGlobalApp should be always <= maxSystem app. But look at this the following way: if the user has 100 queues with maxGlobalApp 1000, and they saturate 10 of them with 10000 applications, then they won't be able to start any application in the 11th queue, because of the maxSystem is reached. So this limitation is already in place. As it should be.
See:
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.
|
💔 -1 overall
This message was automatically generated. |
shuzirra
approved these changes
Oct 18, 2021
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…d a more meaningful name for this method
Description of PR
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?