You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "Galsza (via GitHub)" <gi...@apache.org> on 2024/03/18 11:30:35 UTC
[PR] HDDS-10515. Reduce OzoneAcl constructor count [ozone]
Galsza opened a new pull request, #6396:
URL: https://github.com/apache/ozone/pull/6396
## What changes were proposed in this pull request?
OzoneACL had a few redundant constructors as well as one using BitSet instead of EnumSet. Remove redundancy and make it developer friendlier by using EnumSet
https://issues.apache.org/jira/browse/HDDS-10515
## How was this patch tested?
Locally run some of the modified tests and green CI here:
https://github.com/Galsza/ozone/actions/runs/8316920313
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10515. Reduce OzoneAcl constructor count [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6396:
URL: https://github.com/apache/ozone/pull/6396#discussion_r1531612935
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OzoneAcl.java:
##########
@@ -58,17 +59,12 @@ public class OzoneAcl {
private final AclScope aclScope;
private static final List<ACLType> EMPTY_LIST = new ArrayList<>(0);
- // TODO use varargs constructor
- public OzoneAcl(ACLIdentityType type, String name, ACLType acl, AclScope scope) {
- this(type, name, scope, bitSetOf(acl));
- }
-
public OzoneAcl(ACLIdentityType type, String name, AclScope scope, ACLType... acls) {
this(type, name, scope, bitSetOf(acls));
}
- public OzoneAcl(ACLIdentityType type, String name, BitSet acls, AclScope scope) {
- this(type, name, scope, validateAndCopy(acls));
+ public OzoneAcl(ACLIdentityType type, String name, AclScope scope, EnumSet<ACLType> acls) {
+ this(type, name, scope, bitSetOf(acls.toArray(new ACLType[0])));
Review Comment:
That's how [`toArray(T[])`](https://docs.oracle.com/javase/8/docs/api/java/util/Collection.html#toArray-T:A-) knows the return type. Otherwise [`toArray()`](https://docs.oracle.com/javase/8/docs/api/java/util/Collection.html#toArray--) can only return `Object[]`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10515. Reduce OzoneAcl constructor count [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #6396:
URL: https://github.com/apache/ozone/pull/6396
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10515. Reduce OzoneAcl constructor count [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6396:
URL: https://github.com/apache/ozone/pull/6396#issuecomment-2008939731
Thanks @Galsza for the patch, @dineshchitlangia, @ivandika3 for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10515. Reduce OzoneAcl constructor count [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #6396:
URL: https://github.com/apache/ozone/pull/6396#discussion_r1531442045
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OzoneAcl.java:
##########
@@ -58,17 +59,12 @@ public class OzoneAcl {
private final AclScope aclScope;
private static final List<ACLType> EMPTY_LIST = new ArrayList<>(0);
- // TODO use varargs constructor
- public OzoneAcl(ACLIdentityType type, String name, ACLType acl, AclScope scope) {
- this(type, name, scope, bitSetOf(acl));
- }
-
public OzoneAcl(ACLIdentityType type, String name, AclScope scope, ACLType... acls) {
this(type, name, scope, bitSetOf(acls));
}
- public OzoneAcl(ACLIdentityType type, String name, BitSet acls, AclScope scope) {
- this(type, name, scope, validateAndCopy(acls));
+ public OzoneAcl(ACLIdentityType type, String name, AclScope scope, EnumSet<ACLType> acls) {
+ this(type, name, scope, bitSetOf(acls.toArray(new ACLType[0])));
Review Comment:
The reason of using `toArray(newACLType[0])` is to enforce the runtime type?
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OzoneAcl.java:
##########
@@ -58,17 +59,12 @@ public class OzoneAcl {
private final AclScope aclScope;
private static final List<ACLType> EMPTY_LIST = new ArrayList<>(0);
- // TODO use varargs constructor
- public OzoneAcl(ACLIdentityType type, String name, ACLType acl, AclScope scope) {
- this(type, name, scope, bitSetOf(acl));
- }
-
public OzoneAcl(ACLIdentityType type, String name, AclScope scope, ACLType... acls) {
this(type, name, scope, bitSetOf(acls));
}
- public OzoneAcl(ACLIdentityType type, String name, BitSet acls, AclScope scope) {
- this(type, name, scope, validateAndCopy(acls));
+ public OzoneAcl(ACLIdentityType type, String name, AclScope scope, EnumSet<ACLType> acls) {
+ this(type, name, scope, bitSetOf(acls.toArray(new ACLType[0])));
Review Comment:
The reason of using `toArray(new ACLType[0])` is to enforce the runtime type?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10515. Reduce OzoneAcl constructor count [ozone]
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on PR #6396:
URL: https://github.com/apache/ozone/pull/6396#issuecomment-2007018576
Test failure seems unrelated, it didn't come out locally and the logs didn't point to this fix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10515. Reduce OzoneAcl constructor count [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6396:
URL: https://github.com/apache/ozone/pull/6396#issuecomment-2007084999
@ivandika3 would you like to take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10515. Reduce OzoneAcl constructor count [ozone]
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on PR #6396:
URL: https://github.com/apache/ozone/pull/6396#issuecomment-2006619736
@adoroszlai Thank you for the review comments I've addressed them. PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10515. Reduce OzoneAcl constructor count [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6396:
URL: https://github.com/apache/ozone/pull/6396#discussion_r1528656938
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OzoneAcl.java:
##########
@@ -99,6 +95,12 @@ private static BitSet validateAndCopy(BitSet acls) {
return copyBitSet(acls);
}
+ private static BitSet validateAndCopy(EnumSet<ACLType> enumSet) {
+ BitSet aclBitSet = new BitSet();
+ enumSet.forEach(aclType -> aclBitSet.set(aclType.ordinal()));
+ return validateAndCopy(aclBitSet);
+ }
Review Comment:
- `BitSet` needs to be copied only when provided by external code (one that creates `OzoneAcl`).
- We can reuse `bitSetOf(ACLType...)` to create the `BitSet`. One of the methods can call the other, e.g. `Arrays.asList(acls)` (and declare this method to accept any `Collection<ACLType>`) or `enumSet.toArray(new ACLType[0])`.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OzoneAcl.java:
##########
@@ -170,7 +172,7 @@ public static OzoneAcl parseAcl(String acl)
// TODO : Support sanitation of these user names by calling into
// userAuth Interface.
- return new OzoneAcl(aclType, parts[1], acls, aclScope);
+ return new OzoneAcl(aclType, parts[1], aclScope, validateAndCopy(acls));
Review Comment:
Here `validateAndCopy` can be ommitted, since the `BitSet` is created in `OzoneAcl`. Better yet, construct an `EnumSet`.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -3444,11 +3444,8 @@ public void testNativeAclsForPrefix() throws Exception {
.setStoreType(OzoneObj.StoreType.OZONE)
.build();
- // add acl
- BitSet aclRights1 = new BitSet();
- aclRights1.set(READ.ordinal());
OzoneAcl user1Acl = new OzoneAcl(USER,
- "user1", aclRights1, ACCESS);
+ "user1", EnumSet.of(READ), ACCESS);
Review Comment:
We can avoid wrapping in `EnumSet` if the set is not reused.
```suggestion
"user1", ACCESS, READ);
```
(Also in a few other places, look for inline `EnumSet`.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org