HDDS-10522. Make OmPrefixInfo immutable #9427
Conversation
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @echonesis for the patch.
|
|
||
| public boolean setAcls(List<OzoneAcl> newAcls) { | ||
| return OzoneAclUtil.setAcl(acls, newAcls); | ||
| return ImmutableList.copyOf(acls); |
There was a problem hiding this comment.
nit: since acls is already ImmutableList, we don't need to copy it.
There was a problem hiding this comment.
Yes, you are correct; it is already an ImmutableList.
However, SpotBugs (FindBugs) appears to be mistakenly reporting an issue when detected via:
grep -c 'OmPrefixInfo' target/findbugs/summary.txt
Given this, should we proceed to return acls directly?
There was a problem hiding this comment.
I think that can be fixed by:
private final ImmutableList<OzoneAcl> acls;There was a problem hiding this comment.
Thanks!
This will be included in the next commit.
|
|
||
| assertEquals(omPrefixInfo, clonePrefixInfo); | ||
|
|
||
| OmPrefixInfo modifiedPrefixInfo = new OmPrefixInfo.Builder(omPrefixInfo) |
There was a problem hiding this comment.
nit:
| OmPrefixInfo modifiedPrefixInfo = new OmPrefixInfo.Builder(omPrefixInfo) | |
| OmPrefixInfo modifiedPrefixInfo = omPrefixInfo.toBuilder() |
| assertThrows(UnsupportedOperationException.class, | ||
| () -> omPrefixInfo.getAcls().add(OzoneAcl.of( | ||
| IAccessAuthorizer.ACLIdentityType.USER, username, | ||
| ACCESS, IAccessAuthorizer.ACLType.READ))); |
There was a problem hiding this comment.
nit: this belongs to a separate test case, since it verifies immutability
| } | ||
|
|
||
| boolean removed = prefixInfo.removeAcl(ozoneAcl); | ||
| List<OzoneAcl> updatedAcls = new ArrayList<>(prefixInfo.getAcls()); |
There was a problem hiding this comment.
This copies the list of ACLs, then Builder.setAcls copies them to its own list. I'd like to avoid this double copying. Please see #9430 for proposed improvement, used in OmVolumeArgs as POC. If it's accepted, can be applied to OmPrefixInfo and other classes, too.
| metadataManager.getPrefixTable().put(ozoneObj.getPath(), | ||
| updatedPrefixInfo); |
There was a problem hiding this comment.
nit: no need to wrap (also in removeAcl and setAcl)
| metadataManager.getPrefixTable().put(ozoneObj.getPath(), | |
| updatedPrefixInfo); | |
| metadataManager.getPrefixTable().put(ozoneObj.getPath(), updatedPrefixInfo); |
2 participants
What changes were proposed in this pull request?
This PR makes
OmPrefixInfoimmutable.Essential modifications:
@ImmutabletoOmPrefixInfosetAcls,addAclandremoveAcl.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10522
How was this patch tested?