-
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-11660. Fix huge performance regression for SingleConstraintAppPlacementAllocator #6623
Conversation
💔 -1 overall
This message was automatically generated. |
@@ -309,6 +309,10 @@ private void decreasePendingNumAllocation() { | |||
// Deduct pending #allocations by 1 | |||
ResourceSizing sizing = schedulingRequest.getResourceSizing(); | |||
sizing.setNumAllocations(sizing.getNumAllocations() - 1); | |||
|
|||
appSchedulingInfo.decPendingResource( |
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.
one line may be better.
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.
Done
💔 -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.
+1 LGTM, thanks @zuston for fixing this regression.
@zuston Thanks for the contribution! @dineshchitlangia Thanks for the review! |
Description of PR
JIRA: YARN-11660. Fix huge performance regression for SingleConstraintAppPlacementAllocator.
When using the
SingleConstraintAppPlacementAllocator
with scheduling request in our internal cluster, I found the huge performance regression from the metric ofallocateAvgTime
.After digging this bug, I found this dangerous bug will always check the non-pending resource apps due to missing of desc pending resource for one app for the async scheduling threads.
How was this patch tested?
Has been applied in our internal cluster
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?