This is an automated email from the ASF dual-hosted git repository.
adoroszlai pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ozone.git
The following commit(s) were added to refs/heads/master by this push:
new 17913d99351 HDDS-10522. Make OmPrefixInfo immutable (#9427)
17913d99351 is described below
commit 17913d99351acd5d04fd6be6f35d9b84641fd1e3
Author: Eric C. Ho <[email protected]>
AuthorDate: Wed Dec 10 19:07:39 2025 +0800
HDDS-10522. Make OmPrefixInfo immutable (#9427)
---
hadoop-ozone/interface-storage/pom.xml | 4 +
.../hadoop/ozone/om/helpers/OmPrefixInfo.java | 64 ++++++++-------
.../hadoop/ozone/om/helpers/TestOmPrefixInfo.java | 26 ++++--
.../apache/hadoop/ozone/om/PrefixManagerImpl.java | 92 ++++++++++++----------
4 files changed, 112 insertions(+), 74 deletions(-)
diff --git a/hadoop-ozone/interface-storage/pom.xml
b/hadoop-ozone/interface-storage/pom.xml
index a26ce7f2401..acf48a34e53 100644
--- a/hadoop-ozone/interface-storage/pom.xml
+++ b/hadoop-ozone/interface-storage/pom.xml
@@ -25,6 +25,10 @@
<name>Apache Ozone Storage Interface</name>
<description>Apache Ozone Storage Interface</description>
<dependencies>
+ <dependency>
+ <groupId>com.github.stephenc.jcip</groupId>
+ <artifactId>jcip-annotations</artifactId>
+ </dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
diff --git
a/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/helpers/OmPrefixInfo.java
b/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/helpers/OmPrefixInfo.java
index 36d4e035d6a..20a9fbdd4cf 100644
---
a/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/helpers/OmPrefixInfo.java
+++
b/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/helpers/OmPrefixInfo.java
@@ -17,12 +17,11 @@
package org.apache.hadoop.ozone.om.helpers;
-import java.util.ArrayList;
-import java.util.LinkedList;
+import com.google.common.collect.ImmutableList;
import java.util.List;
import java.util.Map;
import java.util.Objects;
-import java.util.concurrent.CopyOnWriteArrayList;
+import net.jcip.annotations.Immutable;
import org.apache.hadoop.hdds.utils.db.Codec;
import org.apache.hadoop.hdds.utils.db.CopyObject;
import org.apache.hadoop.hdds.utils.db.DelegatedCodec;
@@ -35,6 +34,7 @@
* can be extended for other OzFS optimizations in future.
*/
// TODO: support Auditable interface
+@Immutable
public final class OmPrefixInfo extends WithObjectID implements
CopyObject<OmPrefixInfo> {
private static final Codec<OmPrefixInfo> CODEC = new DelegatedCodec<>(
Proto2Codec.get(PersistedPrefixInfo.getDefaultInstance()),
@@ -43,12 +43,12 @@ public final class OmPrefixInfo extends WithObjectID
implements CopyObject<OmPre
OmPrefixInfo.class);
private final String name;
- private final CopyOnWriteArrayList<OzoneAcl> acls;
+ private final ImmutableList<OzoneAcl> acls;
private OmPrefixInfo(Builder b) {
super(b);
- name = b.name;
- acls = new CopyOnWriteArrayList<>(b.acls);
+ this.name = b.name;
+ this.acls = b.acls.build();
}
public static Codec<OmPrefixInfo> getCodec() {
@@ -63,18 +63,6 @@ public List<OzoneAcl> getAcls() {
return acls;
}
- public boolean addAcl(OzoneAcl acl) {
- return OzoneAclUtil.addAcl(acls, acl);
- }
-
- public boolean removeAcl(OzoneAcl acl) {
- return OzoneAclUtil.removeAcl(acls, acl);
- }
-
- public boolean setAcls(List<OzoneAcl> newAcls) {
- return OzoneAclUtil.setAcl(acls, newAcls);
- }
-
/**
* Returns the name of the prefix path.
* @return name of the prefix path.
@@ -88,8 +76,8 @@ public String getName() {
*
* @return Builder
*/
- public static OmPrefixInfo.Builder newBuilder() {
- return new OmPrefixInfo.Builder();
+ public static Builder newBuilder() {
+ return new Builder();
}
/**
@@ -97,26 +85,47 @@ public static OmPrefixInfo.Builder newBuilder() {
*/
public static class Builder extends WithObjectID.Builder<OmPrefixInfo> {
private String name;
- private final List<OzoneAcl> acls;
+ private final AclListBuilder acls;
public Builder() {
//Default values
- this.acls = new LinkedList<>();
+ this(AclListBuilder.empty());
+ }
+
+ private Builder(AclListBuilder acls) {
+ this.acls = acls;
}
public Builder(OmPrefixInfo obj) {
super(obj);
- setName(obj.name);
- acls = new ArrayList<>(obj.getAcls());
+ this.acls = AclListBuilder.of(obj.acls);
+ this.name = obj.name;
}
public Builder setAcls(List<OzoneAcl> listOfAcls) {
- if (listOfAcls != null) {
- acls.addAll(listOfAcls);
- }
+ acls.set(listOfAcls);
+ return this;
+ }
+
+ public Builder addAcls(List<OzoneAcl> listOfAcls) {
+ acls.addAll(listOfAcls);
+ return this;
+ }
+
+ public Builder addAcl(OzoneAcl acl) {
+ acls.add(acl);
+ return this;
+ }
+
+ public Builder removeAcl(OzoneAcl acl) {
+ acls.remove(acl);
return this;
}
+ public boolean isAclsChanged() {
+ return acls.isChanged();
+ }
+
public Builder setName(String n) {
this.name = n;
return this;
@@ -250,4 +259,3 @@ public Builder toBuilder() {
return new Builder(this);
}
}
-
diff --git
a/hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmPrefixInfo.java
b/hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmPrefixInfo.java
index 5fcaed544af..687136d16e9 100644
---
a/hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmPrefixInfo.java
+++
b/hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmPrefixInfo.java
@@ -20,6 +20,7 @@
import static org.apache.hadoop.ozone.OzoneAcl.AclScope.ACCESS;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import com.google.protobuf.ByteString;
import java.util.ArrayList;
@@ -94,14 +95,29 @@ public void testCopyObject() {
assertEquals(omPrefixInfo, clonePrefixInfo);
+ OmPrefixInfo modifiedPrefixInfo = omPrefixInfo.toBuilder()
+ .addAcls(Collections.singletonList(OzoneAcl.of(
+ IAccessAuthorizer.ACLIdentityType.USER, username,
+ ACCESS, IAccessAuthorizer.ACLType.READ)))
+ .build();
- // Change acls and check.
- omPrefixInfo.addAcl(OzoneAcl.of(
- IAccessAuthorizer.ACLIdentityType.USER, username,
- ACCESS, IAccessAuthorizer.ACLType.READ));
+ assertNotEquals(modifiedPrefixInfo, clonePrefixInfo);
+ }
- assertNotEquals(omPrefixInfo, clonePrefixInfo);
+ @Test
+ public void testImmutability() {
+ String testPath = "/my/custom/path";
+ String username = "myuser";
+ OmPrefixInfo omPrefixInfo = getOmPrefixInfoForTest(testPath,
+ IAccessAuthorizer.ACLIdentityType.USER,
+ username,
+ IAccessAuthorizer.ACLType.WRITE,
+ ACCESS);
+ assertThrows(UnsupportedOperationException.class,
+ () -> omPrefixInfo.getAcls().add(OzoneAcl.of(
+ IAccessAuthorizer.ACLIdentityType.USER, username,
+ ACCESS, IAccessAuthorizer.ACLType.READ)));
}
@Test
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java
index f6615b92f2d..2e1e452aaae 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java
@@ -235,33 +235,35 @@ public OMPrefixAclOpResult addAcl(OzoneObj ozoneObj,
OzoneAcl ozoneAcl,
// No explicit prefix create API, both add/set Acl can get new prefix
// created. When new prefix is created, it should inherit parent prefix
// or bucket default ACLs.
- boolean newPrefix = false;
- if (prefixInfo == null) {
- OmPrefixInfo.Builder prefixInfoBuilder =
- new OmPrefixInfo.Builder()
- .setName(ozoneObj.getPath());
- if (transactionLogIndex > 0) {
- prefixInfoBuilder.setObjectID(OmUtils.getObjectIdFromTxId(
- metadataManager.getOmEpoch(), transactionLogIndex));
- prefixInfoBuilder.setUpdateID(transactionLogIndex);
- }
- prefixInfo = prefixInfoBuilder.build();
- newPrefix = true;
+ boolean newPrefix = prefixInfo == null;
+ OmPrefixInfo.Builder prefixInfoBuilder = newPrefix
+ ? OmPrefixInfo.newBuilder().setName(ozoneObj.getPath())
+ : prefixInfo.toBuilder();
+
+ if (newPrefix && transactionLogIndex > 0) {
+ prefixInfoBuilder.setObjectID(OmUtils.getObjectIdFromTxId(
+ metadataManager.getOmEpoch(), transactionLogIndex));
+ prefixInfoBuilder.setUpdateID(transactionLogIndex);
}
- boolean changed = prefixInfo.addAcl(ozoneAcl);
// Update the in-memory prefix tree regardless whether the ACL is changed.
// Under OM HA, update ID of the prefix info is updated for every request.
if (newPrefix) {
- inheritParentAcl(ozoneObj, prefixInfo);
+ List<OzoneAcl> inheritedAcls = new ArrayList<>();
+ inheritParentAcl(ozoneObj, inheritedAcls);
+ prefixInfoBuilder.addAcls(inheritedAcls);
}
+ prefixInfoBuilder.addAcl(ozoneAcl);
+ boolean changed = prefixInfoBuilder.isAclsChanged();
+
+ OmPrefixInfo updatedPrefixInfo = prefixInfoBuilder.build();
// update the in-memory prefix tree
- prefixTree.insert(ozoneObj.getPath(), prefixInfo);
+ prefixTree.insert(ozoneObj.getPath(), updatedPrefixInfo);
if (!isRatisEnabled) {
- metadataManager.getPrefixTable().put(ozoneObj.getPath(), prefixInfo);
+ metadataManager.getPrefixTable().put(ozoneObj.getPath(),
updatedPrefixInfo);
}
- return new OMPrefixAclOpResult(prefixInfo, changed);
+ return new OMPrefixAclOpResult(updatedPrefixInfo, changed);
}
public OMPrefixAclOpResult removeAcl(OzoneObj ozoneObj, OzoneAcl ozoneAcl,
@@ -270,27 +272,32 @@ public OMPrefixAclOpResult removeAcl(OzoneObj ozoneObj,
OzoneAcl ozoneAcl,
return new OMPrefixAclOpResult(null, false);
}
- boolean removed = prefixInfo.removeAcl(ozoneAcl);
+ OmPrefixInfo.Builder prefixInfoBuilder = prefixInfo.toBuilder();
+ prefixInfoBuilder.removeAcl(ozoneAcl);
+ boolean removed = prefixInfoBuilder.isAclsChanged();
+
+ OmPrefixInfo updatedPrefixInfo = removed
+ ? prefixInfoBuilder.build()
+ : prefixInfo;
// Update in-memory prefix tree regardless whether the ACL is changed.
// Under OM HA, update ID of the prefix info is updated for every request.
- if (prefixInfo.getAcls().isEmpty()) {
+ if (removed && updatedPrefixInfo.getAcls().isEmpty()) {
prefixTree.removePrefixPath(ozoneObj.getPath());
if (!isRatisEnabled) {
metadataManager.getPrefixTable().delete(ozoneObj.getPath());
}
} else {
- prefixTree.insert(ozoneObj.getPath(), prefixInfo);
+ prefixTree.insert(ozoneObj.getPath(), updatedPrefixInfo);
if (!isRatisEnabled) {
- metadataManager.getPrefixTable().put(ozoneObj.getPath(), prefixInfo);
+ metadataManager.getPrefixTable().put(ozoneObj.getPath(),
updatedPrefixInfo);
}
}
- return new OMPrefixAclOpResult(prefixInfo, removed);
+ return new OMPrefixAclOpResult(updatedPrefixInfo, removed);
}
- private void inheritParentAcl(OzoneObj ozoneObj, OmPrefixInfo prefixInfo)
+ private void inheritParentAcl(OzoneObj ozoneObj, List<OzoneAcl> aclsToBeSet)
throws IOException {
- List<OzoneAcl> aclsToBeSet = prefixInfo.getAcls();
// Inherit DEFAULT acls from prefix.
boolean prefixParentFound = false;
List<OmPrefixInfo> prefixList = getLongestPrefixPathHelper(
@@ -319,29 +326,32 @@ private void inheritParentAcl(OzoneObj ozoneObj,
OmPrefixInfo prefixInfo)
public OMPrefixAclOpResult setAcl(OzoneObj ozoneObj, List<OzoneAcl>
ozoneAcls,
OmPrefixInfo prefixInfo, long transactionLogIndex) throws IOException {
- boolean newPrefix = false;
- if (prefixInfo == null) {
- OmPrefixInfo.Builder prefixInfoBuilder =
- new OmPrefixInfo.Builder()
- .setName(ozoneObj.getPath());
- if (transactionLogIndex > 0) {
- prefixInfoBuilder.setObjectID(OmUtils.getObjectIdFromTxId(
- metadataManager.getOmEpoch(), transactionLogIndex));
- prefixInfoBuilder.setUpdateID(transactionLogIndex);
- }
- prefixInfo = prefixInfoBuilder.build();
- newPrefix = true;
+ boolean newPrefix = prefixInfo == null;
+ OmPrefixInfo.Builder prefixInfoBuilder = newPrefix
+ ? OmPrefixInfo.newBuilder().setName(ozoneObj.getPath())
+ : prefixInfo.toBuilder();
+
+ if (newPrefix && transactionLogIndex > 0) {
+ prefixInfoBuilder.setObjectID(OmUtils.getObjectIdFromTxId(
+ metadataManager.getOmEpoch(), transactionLogIndex));
+ prefixInfoBuilder.setUpdateID(transactionLogIndex);
}
- boolean changed = prefixInfo.setAcls(ozoneAcls);
+ List<OzoneAcl> aclsToSet = new ArrayList<>();
+ OzoneAclUtil.setAcl(aclsToSet, ozoneAcls);
if (newPrefix) {
- inheritParentAcl(ozoneObj, prefixInfo);
+ inheritParentAcl(ozoneObj, aclsToSet);
}
- prefixTree.insert(ozoneObj.getPath(), prefixInfo);
+ prefixInfoBuilder.setAcls(aclsToSet);
+ // For new prefix, creation itself is considered a change regardless of
ACL content
+ boolean changed = newPrefix || prefixInfoBuilder.isAclsChanged();
+
+ OmPrefixInfo updatedPrefixInfo = prefixInfoBuilder.build();
+ prefixTree.insert(ozoneObj.getPath(), updatedPrefixInfo);
if (!isRatisEnabled) {
- metadataManager.getPrefixTable().put(ozoneObj.getPath(), prefixInfo);
+ metadataManager.getPrefixTable().put(ozoneObj.getPath(),
updatedPrefixInfo);
}
- return new OMPrefixAclOpResult(prefixInfo, changed);
+ return new OMPrefixAclOpResult(updatedPrefixInfo, changed);
}
/**
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
