-
Notifications
You must be signed in to change notification settings - Fork 9.2k
YARN-10907. Minimize usages of AbstractCSQueue#csContext #3550
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
Conversation
|
💔 -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. |
|
💔 -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. |
|
💔 -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. |
|
💔 -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. |
|
💔 -1 overall
This message was automatically generated. |
9uapaw
left a comment
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.
Thank you @brumi1024 for this massive undertaking, well done! I agree wholeheartedly with this change, however I have two objections on the fundamentals on which it builds.
- I think the context object is not eliminated, its application is rather subtly moved into the QueueManager. Now CSQM knows about nodes, applications and several other things it should not be dealing with. In my opinion this is worse than the csContext approach, as CSQM was more or less a solid class with a single responsibility. This must be addressed either by introducing some real storage class, which is passed to the queues, or by rethinking how to pass individual fields as parameters instead (not sure if it is possible, but this would be the ideal solution).
- I generally prefer composition over inheritance, however the CSQueueProperties concept does not seem to be a real composition pattern. There are 3 concrete classes of CSQueueProperties:
- CSQueueProperties: Applicable to all queues (basically the AbstractCSQueue)
- LeafQueueProperties: Applicable to leaf queues (basically the LeafQueue)
- AutoCreatedLeafQueueProperties: Applicable to legacy dynamic queues (basically AutoCreatedLeafQueues and alike)
As none of these classes uses dynamic dispatch and overridden methods (and you can only use 1 type of property class per queue type eg. ParentQueue can only use CSQueueProperties, LeafQueue can only use CSQueueLeafProperties etc..), composition only complicates the instantiation of the aforementioned queue classes and opens up possibilities of violating their internal behaviour. If possible, these queue type specific actions should be implemented using the existing inheritance hierarchy.
If the queue classes still have too much logic defined in them, we should continue the work started by YARN-10942 and extract the remaining business logic as well.
...adoop/yarn/server/resourcemanager/monitor/capacity/ProportionalCapacityPreemptionPolicy.java
Outdated
Show resolved
Hide resolved
...org/apache/hadoop/yarn/server/resourcemanager/reservation/CapacitySchedulerPlanFollower.java
Outdated
Show resolved
Hide resolved
...org/apache/hadoop/yarn/server/resourcemanager/reservation/CapacitySchedulerPlanFollower.java
Outdated
Show resolved
Hide resolved
...org/apache/hadoop/yarn/server/resourcemanager/reservation/CapacitySchedulerPlanFollower.java
Outdated
Show resolved
Hide resolved
...org/apache/hadoop/yarn/server/resourcemanager/reservation/CapacitySchedulerPlanFollower.java
Outdated
Show resolved
Hide resolved
.../apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CSQueuePreemptionSettings.java
Outdated
Show resolved
Hide resolved
...rg/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueueNodeLabelsSettings.java
Outdated
Show resolved
Hide resolved
.../java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ReservationQueue.java
Outdated
Show resolved
Hide resolved
...hadoop/yarn/server/resourcemanager/scheduler/capacity/TestAbsoluteResourceConfiguration.java
Outdated
Show resolved
Hide resolved
.../org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestApplicationLimits.java
Outdated
Show resolved
Hide resolved
c052565 to
2abb2ec
Compare
|
💔 -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.
Thank you for the revision @brumi1024. Overall the change looks good to me, I only had some minor feedback on it.
.../apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CSQueuePreemptionSettings.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ManagedParentQueue.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ParentQueue.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/UsersManager.java
Outdated
Show resolved
Hide resolved
.../org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestApplicationLimits.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
569e5f4 to
17dd055
Compare
|
💔 -1 overall
This message was automatically generated. |
17dd055 to
90be0dd
Compare
...che/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueContext.java
Show resolved
Hide resolved
...che/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueContext.java
Outdated
Show resolved
Hide resolved
...che/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueContext.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java
Outdated
Show resolved
Hide resolved
...java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractLeafQueue.java
Show resolved
Hide resolved
|
Hi @brumi1024 , |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Hi @brumi1024 , |
Description of PR
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?