[ https://issues.apache.org/jira/browse/YARN-10148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17046397#comment-17046397 ]
Szilard Nemeth edited comment on YARN-10148 at 2/27/20 9:31 AM: ---------------------------------------------------------------- Thanks [~kmarton] for your patch, A couple of comments: *In QueueACLTestBase: * 1. QueueACLsTestBase#checkAccess: This method contains 3 code blocks that are very similar. The method should have a body that contains assertions for administer and submit access: {code} Assert.assertEquals( String.format(failureMsg, QueueACL.ADMINISTER_QUEUE, "root"), rootAccess, resourceManager.getResourceScheduler() .checkAccess(user, QueueACL.ADMINISTER_QUEUE, "root")); Assert.assertEquals( String.format(failureMsg, QueueACL.SUBMIT_APPLICATIONS, "root"), rootAccess, resourceManager.getResourceScheduler() .checkAccess(user, QueueACL.SUBMIT_APPLICATIONS, "root")); {code} I'll let you decide if you want to "hardcode" the String.format call in the extracted method (as it is the same for all 3 calls) or providing it as a parameter. The main point is that the queue name (let it be "root" or a method call that returns the name of the queue like "getQueueD()") should be given as a parameter. You can utilize a Supplier<String> as a parameter: https://www.baeldung.com/java-8-functional-interfaces#Suppliers 2. Can you please add explanation as a javadoc for all the new testcases added to QueueACLTestBase? For me it's not very straightforward and easy to understand them. 3. Nit: In TestCapacitySchedulerQueueACLs#updateConfigWithDAndD1Queues: Please use uppercase "ACL" in the javadoc. 4. In TestCapacitySchedulerQueueACLs#updateConfigWithDAndD1Queues: Local variables cPath, c1Path should be named dPath, d1Path, right? 5. In TestCapacitySchedulerQueueACLs#updateConfigWithDAndD1Queues: To make the whole thing more easy to read, you could extract a helper method that sets a capacity for a queue. {code} csConf.setCapacity(CapacitySchedulerConfiguration.ROOT + "." + QUEUEA, 30f); {code} Having one parameter that could be the name of the leaf queue and the method could have appended the full queue path to it. Of course, you can use the full queue path as a param if you prefer that. 6. A very similar thing to 5. : Could you extract a method that sets the admin and submit ACL together? You are always calling these together: {code} csConf.setAcl(cPath, QueueACL.ADMINISTER_QUEUE, queueDAcl); csConf.setAcl(cPath, QueueACL.SUBMIT_APPLICATIONS, queueDAcl); {code} 7. Nit: In TestCapacitySchedulerQueueACLs#updateConfigWithDAndD1Queues: The if conditions at the end of the method are oddly formatted (no space between if and parentheses). 8. Nit: TestFairSchedulerQueueACLs#updateConfigWithDAndD1Queues: Please use uppercase ACL in the javadoc. was (Author: snemeth): Thanks [~kmarton] for your patch, A couple of comments: Hi Kinga, *In QueueACLTestBase: * 1. QueueACLsTestBase#checkAccess: This method contains 3 code blocks that are very similar. The method should have a body that contains assertions for administer and submit access: {code} Assert.assertEquals( String.format(failureMsg, QueueACL.ADMINISTER_QUEUE, "root"), rootAccess, resourceManager.getResourceScheduler() .checkAccess(user, QueueACL.ADMINISTER_QUEUE, "root")); Assert.assertEquals( String.format(failureMsg, QueueACL.SUBMIT_APPLICATIONS, "root"), rootAccess, resourceManager.getResourceScheduler() .checkAccess(user, QueueACL.SUBMIT_APPLICATIONS, "root")); {code} I'll let you decide if you want to "hardcode" the String.format call in the extracted method (as it is the same for all 3 calls) or providing it as a parameter. The main point is that the queue name (let it be "root" or a method call that returns the name of the queue like "getQueueD()") should be given as a parameter. You can utilize a Supplier<String> as a parameter: https://www.baeldung.com/java-8-functional-interfaces#Suppliers 2. Can you please add explanation as a javadoc for all the new testcases added to QueueACLTestBase? For me it's not very straightforward and easy to understand them. 3. Nit: In TestCapacitySchedulerQueueACLs#updateConfigWithDAndD1Queues: Please use uppercase "ACL" in the javadoc. 4. In TestCapacitySchedulerQueueACLs#updateConfigWithDAndD1Queues: Local variables cPath, c1Path should be named dPath, d1Path, right? 5. In TestCapacitySchedulerQueueACLs#updateConfigWithDAndD1Queues: To make the whole thing more easy to read, you could extract a helper method that sets a capacity for a queue. {code} csConf.setCapacity(CapacitySchedulerConfiguration.ROOT + "." + QUEUEA, 30f); {code} Having one parameter that could be the name of the leaf queue and the method could have appended the full queue path to it. Of course, you can use the full queue path as a param if you prefer that. 6. A very similar thing to 5. : Could you extract a method that sets the admin and submit ACL together? You are always calling these together: {code} csConf.setAcl(cPath, QueueACL.ADMINISTER_QUEUE, queueDAcl); csConf.setAcl(cPath, QueueACL.SUBMIT_APPLICATIONS, queueDAcl); {code} 7. Nit: In TestCapacitySchedulerQueueACLs#updateConfigWithDAndD1Queues: The if conditions at the end of the method are oddly formatted (no space between if and parentheses). 8. Nit: TestFairSchedulerQueueACLs#updateConfigWithDAndD1Queues: Please use uppercase ACL in the javadoc. > Add Unit test for queue ACL for both FS and CS > ---------------------------------------------- > > Key: YARN-10148 > URL: https://issues.apache.org/jira/browse/YARN-10148 > Project: Hadoop YARN > Issue Type: Improvement > Components: scheduler > Reporter: Kinga Marton > Assignee: Kinga Marton > Priority: Major > Attachments: YARN-10148.001.patch, YARN-10148.002.patch, > YARN-10148.003.patch, YARN-10148.004.patch > > > Add some unit tests covering the queue ACL evaluation for both FS and CS. -- This message was sent by Atlassian Jira (v8.3.4#803005) --------------------------------------------------------------------- To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org