[
https://issues.apache.org/jira/browse/YARN-10148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17046397#comment-17046397
]
Szilard Nemeth commented on YARN-10148:
---------------------------------------
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
