YARN-10985. Add some tests to verify ACL behaviour in CapacitySchedulerConfiguration #3570
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
9uapaw
reviewed
Oct 25, 2021
| } | ||
|
|
||
| @Test | ||
| public void testSpecifiedEmptySubmitACLForRootIsNotInherited() { |
There was a problem hiding this comment.
I think this test scenario could be merged with the test beforehand. The empty root ACL and its inheritance property belongs together in my opinion.
There was a problem hiding this comment.
What do you mean? If I have to run those 3 assertions for a different queue path then I'd prefer to keep them in a separate method. The name of the testcase also tells more precisely what is the purpose.
9uapaw
reviewed
Oct 25, 2021
| } | ||
|
|
||
| @Test | ||
| public void testDefaultSubmitACLForRootChildNoneAllowed() { |
There was a problem hiding this comment.
Same for here. See comments below.
9uapaw
reviewed
Oct 25, 2021
There was a problem hiding this comment.
Thanks for the patch @szilard-nemeth. I agree that we need these tests, however we already have a TestCapacitySchedulerQueueACLs test class. It seems odd to have a TestCSConfig class with only ACL tests in it. My opinion is that these tests could belong to TestCapacitySchedulerQueueACLs class instead.
|
Thanks @9uapaw for sharing your opinion.
My new test class (TestCapacitySchedulerConfiguration) is much lightweight and its purpose is to test the CapacitySchedulerConfiguration. The only reason it contains just ACL tests because there were no tests for this class, which is pretty much disappointing. |
szilard-nemeth
force-pushed
the
YARN-10985
branch
from
f967e39
to
b2d027d
Compare
|
To my mind, TestCapacitySchedulerQueueACLs should have testcases which test the functionality of ACLs, like inheritance, groups / user based matching, etc. While the TestCapacitySchedulerConfiguration should test the configuration part, like parsing of different markups of configuration, defaults etc. So I think BOTH tests have merit, and the TestCapacitySchedulerConfiguration class should be extended with other tests, which test the configuration parsing and evaluation. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
9uapaw
approved these changes
Oct 28, 2021
There was a problem hiding this comment.
Thanks @szilard-nemeth for your answers. I can agree to these arguments. I have two additions:
- The ordering of the tests should be revamped. I think the private internal methods should be at the bottom of the file, because it is hard to comprehend this test file at first.
- Could you also add a test case for multiple space ACLs as well? eg. a__b__? As space is the delimiter, I think it is important to cover it as well.
These are only nits, the patch looks good to me +1.
shuzirra
approved these changes
Oct 28, 2021
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…erConfiguration (apache#3570). Contributed by Szilard Nemeth
jojochuang
pushed a commit
to jojochuang/hadoop
that referenced
this pull request
May 23, 2023
…acitySchedulerConfiguration (apache#3570). Contributed by Szilard Nemeth (cherry picked from commit 2e32cc6) Change-Id: I1e4dc3defe02e761f5132e496fc49796510a6135
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?