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 32b16923fe HDDS-10753. OmKeyInfo#acls and WithMetadata#metadata are
not thread safe. (#6591)
32b16923fe is described below
commit 32b16923fea0eb64efd0ca0da1cd5cf9cea78fbe
Author: Tsz-Wo Nicholas Sze <[email protected]>
AuthorDate: Fri Apr 26 00:16:09 2024 -0700
HDDS-10753. OmKeyInfo#acls and WithMetadata#metadata are not thread safe.
(#6591)
---
.../hadoop/ozone/om/helpers/OmBucketInfo.java | 5 +-
.../apache/hadoop/ozone/om/helpers/OmKeyInfo.java | 5 +-
.../hadoop/ozone/om/helpers/OmVolumeArgs.java | 105 ++++++++-------------
.../hadoop/ozone/om/helpers/WithMetadata.java | 8 +-
.../hadoop/ozone/om/helpers/OmPrefixInfo.java | 5 +-
5 files changed, 52 insertions(+), 76 deletions(-)
diff --git
a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketInfo.java
b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketInfo.java
index e58b8e0fe2..5a83f6dbba 100644
---
a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketInfo.java
+++
b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketInfo.java
@@ -23,6 +23,7 @@ import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.concurrent.CopyOnWriteArrayList;
import java.util.stream.Collectors;
import com.google.common.collect.ImmutableList;
@@ -64,7 +65,7 @@ public final class OmBucketInfo extends WithObjectID
implements Auditable, CopyO
/**
* ACL Information (mutable).
*/
- private final List<OzoneAcl> acls;
+ private final CopyOnWriteArrayList<OzoneAcl> acls;
/**
* Bucket Version flag.
*/
@@ -113,7 +114,7 @@ public final class OmBucketInfo extends WithObjectID
implements Auditable, CopyO
super(b);
this.volumeName = b.volumeName;
this.bucketName = b.bucketName;
- this.acls = b.acls;
+ this.acls = new CopyOnWriteArrayList<>(b.acls);
this.isVersionEnabled = b.isVersionEnabled;
this.storageType = b.storageType;
this.creationTime = b.creationTime;
diff --git
a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java
b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java
index 5186dd65fd..59464de69d 100644
---
a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java
+++
b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java
@@ -23,6 +23,7 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.concurrent.CopyOnWriteArrayList;
import com.google.common.collect.ImmutableList;
import org.apache.commons.lang3.StringUtils;
@@ -98,7 +99,7 @@ public final class OmKeyInfo extends WithParentObjectId
/**
* ACL Information.
*/
- private final List<OzoneAcl> acls;
+ private final CopyOnWriteArrayList<OzoneAcl> acls;
private OmKeyInfo(Builder b) {
super(b);
@@ -111,7 +112,7 @@ public final class OmKeyInfo extends WithParentObjectId
this.modificationTime = b.modificationTime;
this.replicationConfig = b.replicationConfig;
this.encInfo = b.encInfo;
- this.acls = b.acls;
+ this.acls = new CopyOnWriteArrayList<>(b.acls);
this.fileChecksum = b.fileChecksum;
this.fileName = b.fileName;
this.isFile = b.isFile;
diff --git
a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmVolumeArgs.java
b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmVolumeArgs.java
index 8eb931410e..499b487836 100644
---
a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmVolumeArgs.java
+++
b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmVolumeArgs.java
@@ -18,11 +18,11 @@
package org.apache.hadoop.ozone.om.helpers;
import java.util.ArrayList;
-import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.concurrent.CopyOnWriteArrayList;
import com.google.common.collect.ImmutableList;
import org.apache.hadoop.hdds.utils.db.Codec;
@@ -60,7 +60,7 @@ public final class OmVolumeArgs extends WithObjectID
private long quotaInBytes;
private long quotaInNamespace;
private long usedNamespace;
- private List<OzoneAcl> acls;
+ private final CopyOnWriteArrayList<OzoneAcl> acls;
/**
* Reference count on this Ozone volume.
*
@@ -75,42 +75,6 @@ public final class OmVolumeArgs extends WithObjectID
*/
private long refCount;
- /**
- * Private constructor, constructed via builder.
- * @param adminName - Administrator's name.
- * @param ownerName - Volume owner's name
- * @param volume - volume name
- * @param quotaInBytes - Volume Quota in bytes.
- * @param quotaInNamespace - Volume Quota in counts.
- * @param usedNamespace - Volume Namespace Quota Usage in counts.
- * @param metadata - metadata map for custom key/value data.
- * @param acls - list of volume acls.
- * @param creationTime - Volume creation time.
- * @param objectID - ID of this object.
- * @param updateID - A sequence number that denotes the last update on this
- * object. This is a monotonically increasing number.
- */
- @SuppressWarnings({"checkstyle:ParameterNumber",
- "This is invoked from a builder."})
- private OmVolumeArgs(String adminName, String ownerName, String volume,
- long quotaInBytes, long quotaInNamespace, long usedNamespace,
- Map<String, String> metadata, List<OzoneAcl> acls, long creationTime,
- long modificationTime, long objectID, long updateID, long refCount) {
- this.adminName = adminName;
- this.ownerName = ownerName;
- this.volume = volume;
- this.quotaInBytes = quotaInBytes;
- this.quotaInNamespace = quotaInNamespace;
- this.usedNamespace = usedNamespace;
- setMetadata(metadata);
- this.acls = acls;
- this.creationTime = creationTime;
- this.modificationTime = modificationTime;
- setObjectID(objectID);
- setUpdateID(updateID);
- this.refCount = refCount;
- }
-
private OmVolumeArgs(Builder b) {
super(b);
this.adminName = b.adminName;
@@ -119,7 +83,7 @@ public final class OmVolumeArgs extends WithObjectID
this.quotaInBytes = b.quotaInBytes;
this.quotaInNamespace = b.quotaInNamespace;
this.usedNamespace = b.usedNamespace;
- this.acls = b.acls;
+ this.acls = new CopyOnWriteArrayList<>(b.acls);
this.creationTime = b.creationTime;
this.modificationTime = b.modificationTime;
this.refCount = b.refCount;
@@ -320,7 +284,7 @@ public final class OmVolumeArgs extends WithObjectID
private long quotaInBytes;
private long quotaInNamespace;
private long usedNamespace;
- private List<OzoneAcl> acls;
+ private final List<OzoneAcl> acls;
private long refCount;
@Override
@@ -339,7 +303,11 @@ public final class OmVolumeArgs extends WithObjectID
* Constructs a builder.
*/
public Builder() {
- acls = new ArrayList<>();
+ this(new ArrayList<>());
+ }
+
+ private Builder(List<OzoneAcl> acls) {
+ this.acls = acls;
quotaInBytes = OzoneConsts.QUOTA_RESET;
quotaInNamespace = OzoneConsts.QUOTA_RESET;
}
@@ -401,10 +369,11 @@ public final class OmVolumeArgs extends WithObjectID
return this;
}
- public void setRefCount(long refCount) {
+ public Builder setRefCount(long refCount) {
Preconditions.checkState(refCount >= 0L,
"refCount should not be negative");
this.refCount = refCount;
+ return this;
}
public OmVolumeArgs build() {
@@ -437,22 +406,20 @@ public final class OmVolumeArgs extends WithObjectID
}
public static OmVolumeArgs getFromProtobuf(VolumeInfo volInfo) {
- List<OzoneAcl> acls = OzoneAclUtil.fromProtobuf(
- volInfo.getVolumeAclsList());
- return new OmVolumeArgs(
- volInfo.getAdminName(),
- volInfo.getOwnerName(),
- volInfo.getVolume(),
- volInfo.getQuotaInBytes(),
- volInfo.getQuotaInNamespace(),
- volInfo.getUsedNamespace(),
- KeyValueUtil.getFromProtobuf(volInfo.getMetadataList()),
- acls,
- volInfo.getCreationTime(),
- volInfo.getModificationTime(),
- volInfo.getObjectID(),
- volInfo.getUpdateID(),
- volInfo.getRefCount());
+ return new Builder(OzoneAclUtil.fromProtobuf(volInfo.getVolumeAclsList()))
+ .setAdminName(volInfo.getAdminName())
+ .setOwnerName(volInfo.getOwnerName())
+ .setVolume(volInfo.getVolume())
+ .setQuotaInBytes(volInfo.getQuotaInBytes())
+ .setQuotaInNamespace(volInfo.getQuotaInNamespace())
+ .setUsedNamespace(volInfo.getUsedNamespace())
+
.addAllMetadata(KeyValueUtil.getFromProtobuf(volInfo.getMetadataList()))
+ .setCreationTime(volInfo.getCreationTime())
+ .setModificationTime(volInfo.getModificationTime())
+ .setObjectID(volInfo.getObjectID())
+ .setUpdateID(volInfo.getUpdateID())
+ .setRefCount(volInfo.getRefCount())
+ .build();
}
@Override
@@ -470,13 +437,19 @@ public final class OmVolumeArgs extends WithObjectID
@Override
public OmVolumeArgs copyObject() {
- Map<String, String> cloneMetadata = new HashMap<>();
- if (getMetadata() != null) {
- cloneMetadata.putAll(getMetadata());
- }
-
- return new OmVolumeArgs(adminName, ownerName, volume, quotaInBytes,
- quotaInNamespace, usedNamespace, cloneMetadata, new ArrayList<>(acls),
- creationTime, modificationTime, getObjectID(), getUpdateID(),
refCount);
+ return new Builder(acls)
+ .setAdminName(adminName)
+ .setOwnerName(ownerName)
+ .setVolume(volume)
+ .setQuotaInBytes(quotaInBytes)
+ .setQuotaInNamespace(quotaInNamespace)
+ .setUsedNamespace(usedNamespace)
+ .addAllMetadata(getMetadata())
+ .setCreationTime(creationTime)
+ .setModificationTime(modificationTime)
+ .setObjectID(getObjectID())
+ .setUpdateID(getUpdateID())
+ .setRefCount(refCount)
+ .build();
}
}
diff --git
a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithMetadata.java
b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithMetadata.java
index c0481c212e..0993e9a4cd 100644
---
a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithMetadata.java
+++
b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithMetadata.java
@@ -17,8 +17,8 @@
*/
package org.apache.hadoop.ozone.om.helpers;
-import java.util.HashMap;
import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
/**
* Mixin class to handle custom metadata.
@@ -28,7 +28,7 @@ public abstract class WithMetadata {
private Map<String, String> metadata;
protected WithMetadata() {
- metadata = new HashMap<>();
+ metadata = new ConcurrentHashMap<>();
}
protected WithMetadata(Builder b) {
@@ -54,11 +54,11 @@ public abstract class WithMetadata {
private final Map<String, String> metadata;
protected Builder() {
- metadata = new HashMap<>();
+ metadata = new ConcurrentHashMap<>();
}
protected Builder(WithObjectID obj) {
- metadata = new HashMap<>(obj.getMetadata());
+ metadata = new ConcurrentHashMap<>(obj.getMetadata());
}
public Builder addMetadata(String key, String value) {
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 11e98e3ad5..30fe6d69b7 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
@@ -31,6 +31,7 @@ import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.concurrent.CopyOnWriteArrayList;
/**
* Wrapper class for Ozone prefix path info, currently mainly target for ACL
but
@@ -48,12 +49,12 @@ public final class OmPrefixInfo extends WithObjectID
implements CopyObject<OmPre
}
private final String name;
- private final List<OzoneAcl> acls;
+ private final CopyOnWriteArrayList<OzoneAcl> acls;
private OmPrefixInfo(Builder b) {
super(b);
name = b.name;
- acls = new ArrayList<>(b.acls);
+ acls = new CopyOnWriteArrayList<>(b.acls);
}
/**
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
