-
Notifications
You must be signed in to change notification settings - Fork 9.1k
YARN-11069. Dynamic Queue ACL handling in Legacy and Flexible Auto Created Queues #3938
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
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 for the changes @tomicooler. Generally the patch looks good to me, only had some non critical feedback on it.
...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
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
Outdated
Show resolved
Hide resolved
...esourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
Outdated
Show resolved
Hide resolved
...esourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
...g/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AutoCreatedQueueTemplate.java
Outdated
Show resolved
Hide resolved
f6ac78e to
c003572
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. |
|
💔 -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 changes @tomicooler. Had some additional feedbacks. Also I was wondering whether we should centralise ACL handling thus eliminating the need for this double bookkeeping (because of dynamic queues we are checking the ACLs before they are created, and then storing the actual ACLs in them as well). I think an ACLManager would be appropriate. What do you think?
...che/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java
Outdated
Show resolved
Hide resolved
...esourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
Outdated
Show resolved
Hide resolved
...esourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
Outdated
Show resolved
Hide resolved
...esourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
Outdated
Show resolved
Hide resolved
...esourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
Outdated
Show resolved
Hide resolved
...che/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java
Outdated
Show resolved
Hide resolved
|
@9uapaw about the ACLManagers: ApplicationACLsManager There are already some, and they are all have some (serious) design problems. Since queues may not exists yet when we need to check ACLs for them, it's probably not a good idea to store the ACLs in the queues. Anyway, I don't think I have enough expertise on YARN to make big refactors in this context. Even if we want to it should be in a separate PR. |
|
@tomicooler Thanks for the research, of course I did not mean that the problem should be addressed in this exact PR. We could, however, create a JIRA for it and revisit it later. |
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 for the changes @tomicooler, no more comments from my side +1.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
285dd8e to
9ffcb09
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
05791ab to
a191fb5
Compare
|
🎊 +1 overall
This message was automatically generated. |
a191fb5 to
0de158c
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
@tomicooler there seems to be a conflict, can you please resolve it? |
…ic ACL handling. Change-Id: I82e0813ce632064e9ea65593489026e443731f89
…xible AQC. Change-Id: Ib8192127d43119685e4c26ac0c6d8846da02c35b
Change-Id: I48e6fe74bd98c647229c7206846e04c8c7079f2c
Change-Id: I5f8ed67232ab898df4ea3c0e92d45eb5fd169ec2
Change-Id: I6bff52a2b8e5a491308643a2e5a47f03b98d7a61
Change-Id: Ia59d98ee7bb317b3797d5868d55d1dcc9cdb9a35
Change-Id: Ic69a882026ae8d0fc7c49b0e9d95db4734612d2e
Change-Id: Ib6821984167027098d3382da5b4b7b9c0763447c
Change-Id: I59099251a3c25a02521cfc0525da4a9e63dc25a1
Change-Id: I7db3e58b4b13bbf53f31e2ca9998499c010d6daf
Change-Id: I63487bc33892d379f14bac786a8797396c969cd6
Change-Id: If2f1a8df0df06f7ef69fdb7f0e94b7c406f2fb92
Change-Id: Idf4d87b6952d9cc466b346178d978853c0083a2c
Change-Id: I2eadbcf43a7703a4a5c9f95c5ad72ddf9c421124
Change-Id: I113aa0a31c432b58fb9024baa69a4d4a0b1a9d00
…ing the acls would be a must also fixes the currentUser -> userUgi and the empty populateDynamicQueueACLProperties is removed (YarnAuthorizationProvider/ConfiguredYarnAuthorizer is a bad design btw) Change-Id: I63eb7d8dbe7e95d54e6d3e1747319398e6bcc628
…e) type. Change-Id: I208cca91b536e01cf68224f960a34b7a8a5101e7
Change-Id: I99080fd905bfecea4185ee9fcec8167d83b73596
Change-Id: Iaa94dbbe832414842f15716b018c308023993195
Change-Id: I1dd1e2067d4a3e809f9fbe0089db0cdf53ea7c40
0de158c to
dc405be
Compare
|
@brumi1024 I rebased to trunk and resolved the conflicts. |
|
💔 -1 overall
This message was automatically generated. |
Change-Id: Ia08e6d9f9b14384d0356e4fba720a3ec66076e44
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks @tomicooler for working on this, and @9uapaw for the review. Committed to trunk. |
|
🎊 +1 overall
This message was automatically generated. |
No description provided.