-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
YARN-10965. Centralize queue resource calculation based on CapacityVectors #3470
Conversation
(cherry picked from commit efc535e)
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
…ulate the intermediate values that belong together
💔 -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @9uapaw for this huge patch! The logic in general looks good to me, it's nicely done, however I haven't finished going through all of it in detail. In the meantime I posted a few minor comments.
...p-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/util/resource/ResourceUtils.java
Outdated
Show resolved
Hide resolved
...p-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/util/resource/ResourceUtils.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java
Outdated
Show resolved
Hide resolved
...adoop/yarn/server/resourcemanager/scheduler/capacity/AbsoluteResourceCapacityCalculator.java
Show resolved
Hide resolved
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished going through it. Apart from my minor comments I think this change looks good, it makes the capacity calculation more flexible.
...in/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ResourceVector.java
Outdated
Show resolved
Hide resolved
...hadoop/yarn/server/resourcemanager/scheduler/capacity/TestMixedQueueResourceCalculation.java
Show resolved
Hide resolved
...hadoop/yarn/server/resourcemanager/scheduler/capacity/TestMixedQueueResourceCalculation.java
Show resolved
Hide resolved
...rc/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/LeafQueue.java
Outdated
Show resolved
Hide resolved
...p-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/util/resource/ResourceUtils.java
Outdated
Show resolved
Hide resolved
...p-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/util/resource/ResourceUtils.java
Outdated
Show resolved
Hide resolved
...p-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/util/resource/ResourceUtils.java
Outdated
Show resolved
Hide resolved
...in/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ResourceVector.java
Outdated
Show resolved
Hide resolved
...op/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueCapacityHandler.java
Outdated
Show resolved
Hide resolved
.../apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ResourceCalculationDriver.java
Outdated
Show resolved
Hide resolved
.../apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ResourceCalculationDriver.java
Outdated
Show resolved
Hide resolved
sumWeightsPerLabel.get(label).put(resourceName, | ||
sumWeightsPerLabel.get(label).getOrDefault(resourceName, 0f) + value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is called frequently we might consider using a mutable float here to reduce GC strain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@9uapaw Is this comment fixed?
...e/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractQueueCapacityCalculator.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the changes you made since my last request, and I think all of them are fine. I have a few suggestions to clean up the terminology, I think we are using the "child queue" terminology far too often, even in cases when we are only working on a single queue. But these are nice to have suggestions only.
I want to review the whole path as a whole again, especially focusing on the calculation parts, but if I don't find anythin there, I'll give you a +1. (However since the patch is complex, I'd like @szilard-nemeth to look at it as well before we merge it)
...e/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractQueueCapacityCalculator.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CalculationContext.java
Outdated
Show resolved
Hide resolved
...op/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueCapacityHandler.java
Outdated
Show resolved
Hide resolved
.../apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ResourceCalculationDriver.java
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@9uapaw thank you for the update, and for this big patch LGTM+1. But since this is a large patch, I'd like to wait for @szilard-nemeth 's review as well, before we move on to merging this in.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
...che/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java
Show resolved
Hide resolved
...op/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueCapacityHandler.java
Show resolved
Hide resolved
sumWeightsPerLabel.get(label).put(resourceName, | ||
sumWeightsPerLabel.get(label).getOrDefault(resourceName, 0f) + value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@9uapaw Is this comment fixed?
Hi @9uapaw , Thanks. |
@9uapaw Also, we need a fresh Jenkins build so a rebase (or empty commit) is required. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Added commit that fixes checkstyle issues.
LGTM +1, waiting for Jenkins |
💔 -1 overall
This message was automatically generated. |
Thanks @9uapaw for this huge contribution. |
Description of PR
How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?