HDDS-10454. Make OzoneAcl immutable#6319
Conversation
|
@ChenSammi @ivandika3 @smengcl @whbing please take a look |
ivandika3
left a comment
ivandika3
left a comment
There was a problem hiding this comment.
Thanks for the OzoneAcl improvements. I have left some minor comments.
| * Default constructor. | ||
| */ | ||
| public OzoneAcl() { | ||
| // TODO use varargs constructor |
There was a problem hiding this comment.
Can this be done in this patch? Seems the other constructors already using varargs.
If there are a lot of codes that construct the OzoneAcl this way, perhaps we can defer this to another patch.
There was a problem hiding this comment.
Yes, I wanted to defer this to another PR, because there are 70 callers, so it would inflate the patch.
| * OzoneUtils.addAcl(). | ||
| * @param acls | ||
| * @param parentAcls | ||
| * @param scope |
There was a problem hiding this comment.
Thanks for the logic simplification. Could you help update the Javadoc for inheritDefaultAcls?
Helper function to inherit default ACL as access ACL for child object
Is outdated since the inherited ACLs can be of DEFAULT scope (for directory ACLs)
| } | ||
|
|
||
| public boolean checkAccess(ACLType acl) { | ||
| return ((aclBitSet.get(acl.ordinal()) |
There was a problem hiding this comment.
Nit: let's reuse isSet for this
There was a problem hiding this comment.
Also let's add @VisibleForTesting for isSet since it seems to be only used in tests.
ivandika3
left a comment
ivandika3
left a comment
There was a problem hiding this comment.
Thank you for updating the patch. LGTM +1.
| case USER: | ||
| if (a.getName().equals(ugi.getShortUserName())) { | ||
| return checkIfAclBitIsSet(aclToCheck, rights); | ||
| return a.checkAccess(aclToCheck); |
There was a problem hiding this comment.
NIT: checkIfAclBitIsSet method can be removed as it not in used now.
adoroszlai
added
the
code-cleanup
adoroszlai
requested review from
ChenSammi,
aswinshakil,
fapifta and
whbing
and removed request for
whbing
There was a problem hiding this comment.
The patch LGTM. Thanks @adoroszlai , and @whbing @ivandika3 for the review.
|
Thanks @ChenSammi, @ivandika3, @whbing for the review. |
4 participants
What changes were proposed in this pull request?
Make
OzoneAclimmutable. ItsaclBitSetis a built-in mutable object, so avoid exposing it. Instead provide methods for read access, as well as copyingOzoneAclwith changed bits or scope.Draft because #6268 would introduce conflicts with this one.https://issues.apache.org/jira/browse/HDDS-10454
How was this patch tested?
Refactoring, covered by existing tests.
https://github.com/adoroszlai/ozone/actions/runs/8129619085