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-10953. Make CapacityScheduler#getOrCreateQueueFromPlacementConte… #3475
Conversation
…xt easier to comprehend
💔 -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 working on this, non-binding +1.
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. I think the existing test coverage should be enough for this.
...java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacityScheduler.java
Outdated
Show resolved
Hide resolved
...java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacityScheduler.java
Outdated
Show resolved
Hide resolved
...java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacityScheduler.java
Outdated
Show resolved
Hide resolved
...java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacityScheduler.java
Outdated
Show resolved
Hide resolved
|
||
try { | ||
writeLock.lock(); | ||
return queueManager.createQueue(fallbackContext); |
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.
We need to make a polymorphic version of this method (actually two, one with the common parts, which accepts two strings, and one which accepts the QueuePath object) which accepts the QueuePath object, internally this method only uses parent / leaf names, and since we want to use QueuePath object more often, it is a good opportunity, to start adding support for it.
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.
Actually I have replaced all references of ApplicationContext to QueuePath in QueueManager. I did not find any usage that would justify an overloaded method for this, but I presume it could be handy for PlacementRules maybe?
...java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacityScheduler.java
Outdated
Show resolved
Hide resolved
ApplicationId applicationId, String user, String queueName, | ||
boolean isRecovery, Exception e) { | ||
if (isRecovery) { | ||
if (!getConfiguration().shouldAppFailFast(getConfig())) { |
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.
Couldn't we save it as a member variable, it seems a bit unnecessary to have 3 method calls, and a map lookup, just for getting a simple flag. We might need to make sure that the flag is recalculated on config reload tho.
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.
Also found that shouldAppFailFast is a strange method, that could be either:
- Defined only in the config class without a parameter
- Defined as a static method
I have chosen the latter, as I am not sure about how each configuration is used in CS (there are 3 of them) and I would refrain from altering it as of now (it could lead to really subtle issues).
...java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacityScheduler.java
Show resolved
Hide resolved
...java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacityScheduler.java
Show resolved
Hide resolved
🎊 +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 patch, LGTM+1.
🎊 +1 overall
This message was automatically generated. |
…xt easier to comprehend
Description of PR
How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?