From 7ada17cb6b8ee720b881f4fe724730d8bfbe74cc Mon Sep 17 00:00:00 2001 From: "Doroszlai, Attila" Date: Wed, 7 Feb 2024 21:55:12 +0100 Subject: [PATCH] HDDS-10325. Make BucketArgs immutable --- .../hadoop/ozone/client/BucketArgs.java | 94 ++++++++----------- .../hadoop/ozone/client/rpc/RpcClient.java | 3 +- .../org/apache/hadoop/ozone/OzoneAcl.java | 16 +++- .../AbstractRootedOzoneFileSystemTest.java | 15 +-- .../rpc/TestOzoneRpcClientAbstract.java | 43 ++++----- 5 files changed, 74 insertions(+), 97 deletions(-) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/BucketArgs.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/BucketArgs.java index 687605987a68..fee94c55f9ab 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/BucketArgs.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/BucketArgs.java @@ -18,12 +18,15 @@ package org.apache.hadoop.ozone.client; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import org.apache.hadoop.hdds.client.DefaultReplicationConfig; import org.apache.hadoop.hdds.protocol.StorageType; import org.apache.hadoop.ozone.OzoneAcl; import org.apache.hadoop.ozone.OzoneConsts; import org.apache.hadoop.ozone.om.helpers.BucketLayout; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -37,80 +40,60 @@ public final class BucketArgs { /** * ACL Information. */ - private List acls; + private final ImmutableList acls; /** * Bucket Version flag. */ - private Boolean versioning; + private final boolean versioning; /** * Type of storage to be used for this bucket. * [RAM_DISK, SSD, DISK, ARCHIVE] */ - private StorageType storageType; + private final StorageType storageType; /** * Custom key/value metadata. */ - private Map metadata; + private final Map metadata; /** * Bucket encryption key name. */ - private String bucketEncryptionKey; - private DefaultReplicationConfig defaultReplicationConfig; + private final String bucketEncryptionKey; + private final DefaultReplicationConfig defaultReplicationConfig; private final String sourceVolume; private final String sourceBucket; - private long quotaInBytes; - private long quotaInNamespace; + private final long quotaInBytes; + private final long quotaInNamespace; - private String owner; + private final String owner; /** * Bucket Layout. */ - private BucketLayout bucketLayout = BucketLayout.DEFAULT; - - /** - * Private constructor, constructed via builder. - * @param versioning Bucket version flag. - * @param storageType Storage type to be used. - * @param acls list of ACLs. - * @param metadata map of bucket metadata - * @param bucketEncryptionKey bucket encryption key name - * @param sourceVolume - * @param sourceBucket - * @param quotaInBytes Bucket quota in bytes. - * @param quotaInNamespace Bucket quota in counts. - * @param bucketLayout bucket layout. - * @param owner owner of the bucket. - * @param defaultReplicationConfig default replication config. - */ - @SuppressWarnings("parameternumber") - private BucketArgs(Boolean versioning, StorageType storageType, - List acls, Map metadata, - String bucketEncryptionKey, String sourceVolume, String sourceBucket, - long quotaInBytes, long quotaInNamespace, BucketLayout bucketLayout, - String owner, DefaultReplicationConfig defaultReplicationConfig) { - this.acls = acls; - this.versioning = versioning; - this.storageType = storageType; - this.metadata = metadata; - this.bucketEncryptionKey = bucketEncryptionKey; - this.sourceVolume = sourceVolume; - this.sourceBucket = sourceBucket; - this.quotaInBytes = quotaInBytes; - this.quotaInNamespace = quotaInNamespace; - this.bucketLayout = bucketLayout; - this.owner = owner; - this.defaultReplicationConfig = defaultReplicationConfig; + private final BucketLayout bucketLayout; + + private BucketArgs(Builder b) { + acls = b.acls == null ? ImmutableList.of() : ImmutableList.copyOf(b.acls); + versioning = b.versioning; + storageType = b.storageType; + metadata = b.metadata == null ? ImmutableMap.of() : ImmutableMap.copyOf(b.metadata); + bucketEncryptionKey = b.bucketEncryptionKey; + sourceVolume = b.sourceVolume; + sourceBucket = b.sourceBucket; + quotaInBytes = b.quotaInBytes; + quotaInNamespace = b.quotaInNamespace; + bucketLayout = b.bucketLayout; + owner = b.owner; + defaultReplicationConfig = b.defaultReplicationConfig; } /** * Returns true if bucket version is enabled, else false. * @return isVersionEnabled */ - public Boolean getVersioning() { + public boolean getVersioning() { return versioning; } @@ -206,7 +189,7 @@ public String getOwner() { * Builder for OmBucketInfo. */ public static class Builder { - private Boolean versioning; + private boolean versioning; private StorageType storageType; private List acls; private Map metadata; @@ -220,12 +203,11 @@ public static class Builder { private DefaultReplicationConfig defaultReplicationConfig; public Builder() { - metadata = new HashMap<>(); quotaInBytes = OzoneConsts.QUOTA_RESET; quotaInNamespace = OzoneConsts.QUOTA_RESET; } - public BucketArgs.Builder setVersioning(Boolean versionFlag) { + public BucketArgs.Builder setVersioning(boolean versionFlag) { this.versioning = versionFlag; return this; } @@ -235,13 +217,19 @@ public BucketArgs.Builder setStorageType(StorageType storage) { return this; } - public BucketArgs.Builder setAcls(List listOfAcls) { - this.acls = listOfAcls; + public BucketArgs.Builder addAcl(OzoneAcl acl) { + if (acls == null) { + acls = new ArrayList<>(); + } + acls.add(acl); return this; } public BucketArgs.Builder addMetadata(String key, String value) { - this.metadata.put(key, value); + if (metadata == null) { + metadata = new HashMap<>(); + } + metadata.put(key, value); return this; } @@ -291,9 +279,7 @@ public BucketArgs.Builder setDefaultReplicationConfig( * @return instance of BucketArgs. */ public BucketArgs build() { - return new BucketArgs(versioning, storageType, acls, metadata, - bucketEncryptionKey, sourceVolume, sourceBucket, quotaInBytes, - quotaInNamespace, bucketLayout, owner, defaultReplicationConfig); + return new BucketArgs(this); } } } diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java index 8343b8740169..b3d853be6b59 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java @@ -627,8 +627,7 @@ public void createBucket( ugi.getShortUserName() : bucketArgs.getOwner(); } - Boolean isVersionEnabled = bucketArgs.getVersioning() == null ? - Boolean.FALSE : bucketArgs.getVersioning(); + boolean isVersionEnabled = bucketArgs.getVersioning(); StorageType storageType = bucketArgs.getStorageType() == null ? StorageType.DEFAULT : bucketArgs.getStorageType(); BucketLayout bucketLayout = bucketArgs.getBucketLayout(); diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OzoneAcl.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OzoneAcl.java index 7ca0634949c0..2b79d24fd953 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OzoneAcl.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OzoneAcl.java @@ -97,9 +97,23 @@ public OzoneAcl(ACLIdentityType type, String name, ACLType acl, * * @param type - Type * @param name - Name of user - * @param acls - Rights * @param scope - AclScope + * @param acls - Rights */ + public OzoneAcl(ACLIdentityType type, String name, AclScope scope, ACLType... acls) { + this(type, name, bitSetOf(acls), scope); + } + + private static BitSet bitSetOf(ACLType... acls) { + BitSet bits = new BitSet(); + if (acls != null && acls.length > 0) { + for (ACLType acl : acls) { + bits.set(acl.ordinal()); + } + } + return bits; + } + public OzoneAcl(ACLIdentityType type, String name, BitSet acls, AclScope scope) { Objects.requireNonNull(type); diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractRootedOzoneFileSystemTest.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractRootedOzoneFileSystemTest.java index 8ee82633d59a..03167ce11f36 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractRootedOzoneFileSystemTest.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractRootedOzoneFileSystemTest.java @@ -90,7 +90,6 @@ import java.util.Arrays; import java.util.BitSet; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.LinkedList; @@ -122,7 +121,6 @@ import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.VOLUME_NOT_FOUND; import static org.apache.hadoop.ozone.security.acl.IAccessAuthorizer.ACLType.READ; import static org.apache.hadoop.ozone.security.acl.IAccessAuthorizer.ACLType.WRITE; -import static org.apache.hadoop.ozone.security.acl.IAccessAuthorizer.ACLType.DELETE; import static org.apache.hadoop.ozone.security.acl.IAccessAuthorizer.ACLType.LIST; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertArrayEquals; @@ -1223,20 +1221,11 @@ void testSharedTmpDir() throws IOException { } // set acls for shared tmp mount under the tmp volume - List objectAcls = new ArrayList<>(); - objectAcls.add(new OzoneAcl(ACLIdentityType.USER, "admin", userRights, - ACCESS)); - aclRights.clear(DELETE.ordinal()); - aclRights.set(LIST.ordinal()); - objectAcls.add(new OzoneAcl(ACLIdentityType.WORLD, "", - aclRights, ACCESS)); - objectAcls.add(new OzoneAcl(ACLIdentityType.USER, "admin", userRights, - ACCESS)); // bucket acls have all access to admin and read+write+list access to world - BucketArgs bucketArgs = new BucketArgs.Builder() .setOwner("admin") - .setAcls(Collections.unmodifiableList(objectAcls)) + .addAcl(new OzoneAcl(ACLIdentityType.WORLD, "", ACCESS, READ, WRITE, LIST)) + .addAcl(new OzoneAcl(ACLIdentityType.USER, "admin", userRights, ACCESS)) .setQuotaInNamespace(1000) .setQuotaInBytes(Long.MAX_VALUE).build(); diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java index 98bf65ad6b6f..28697379072e 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java @@ -661,12 +661,10 @@ public void testCreateBucketWithAcls() String bucketName = UUID.randomUUID().toString(); OzoneAcl userAcl = new OzoneAcl(USER, "test", READ, ACCESS); - List acls = new ArrayList<>(); - acls.add(userAcl); store.createVolume(volumeName); OzoneVolume volume = store.getVolume(volumeName); - BucketArgs.Builder builder = BucketArgs.newBuilder(); - builder.setAcls(acls); + BucketArgs.Builder builder = BucketArgs.newBuilder() + .addAcl(userAcl); volume.createBucket(bucketName, builder.build()); OzoneBucket bucket = volume.getBucket(bucketName); assertEquals(bucketName, bucket.getName()); @@ -697,15 +695,13 @@ public void testCreateBucketWithAllArgument() String bucketName = UUID.randomUUID().toString(); OzoneAcl userAcl = new OzoneAcl(USER, "test", ACLType.ALL, ACCESS); - List acls = new ArrayList<>(); - acls.add(userAcl); ReplicationConfig repConfig = new ECReplicationConfig(3, 2); store.createVolume(volumeName); OzoneVolume volume = store.getVolume(volumeName); BucketArgs.Builder builder = BucketArgs.newBuilder(); builder.setVersioning(true) .setStorageType(StorageType.SSD) - .setAcls(acls) + .addAcl(userAcl) .setDefaultReplicationConfig(new DefaultReplicationConfig(repConfig)); volume.createBucket(bucketName, builder.build()); OzoneBucket bucket = volume.getBucket(bucketName); @@ -754,20 +750,16 @@ public void testRemoveBucketAcl() String bucketName = UUID.randomUUID().toString(); OzoneAcl userAcl = new OzoneAcl(USER, "test", ACLType.ALL, ACCESS); - List acls = new ArrayList<>(); - acls.add(userAcl); store.createVolume(volumeName); OzoneVolume volume = store.getVolume(volumeName); - BucketArgs.Builder builder = BucketArgs.newBuilder(); - builder.setAcls(acls); + BucketArgs.Builder builder = BucketArgs.newBuilder() + .addAcl(userAcl); volume.createBucket(bucketName, builder.build()); OzoneBucket bucket = volume.getBucket(bucketName); - for (OzoneAcl acl : acls) { - assertTrue(bucket.removeAcl(acl)); - } + assertTrue(bucket.removeAcl(userAcl)); OzoneBucket newBucket = volume.getBucket(bucketName); assertEquals(bucketName, newBucket.getName()); - assertThat(bucket.getAcls()).doesNotContain(acls.get(0)); + assertThat(newBucket.getAcls()).doesNotContain(userAcl); } @Test @@ -777,14 +769,13 @@ public void testRemoveBucketAclUsingRpcClientRemoveAcl() String bucketName = UUID.randomUUID().toString(); OzoneAcl userAcl = new OzoneAcl(USER, "test", ACLType.ALL, ACCESS); - List acls = new ArrayList<>(); - acls.add(userAcl); - acls.add(new OzoneAcl(USER, "test1", - ACLType.ALL, ACCESS)); + OzoneAcl acl2 = new OzoneAcl(USER, "test1", + ACLType.ALL, ACCESS); store.createVolume(volumeName); OzoneVolume volume = store.getVolume(volumeName); - BucketArgs.Builder builder = BucketArgs.newBuilder(); - builder.setAcls(acls); + BucketArgs.Builder builder = BucketArgs.newBuilder() + .addAcl(userAcl) + .addAcl(acl2); volume.createBucket(bucketName, builder.build()); OzoneObj ozoneObj = OzoneObjInfo.Builder.newBuilder() .setBucketName(bucketName) @@ -793,13 +784,11 @@ public void testRemoveBucketAclUsingRpcClientRemoveAcl() .setResType(OzoneObj.ResourceType.BUCKET).build(); // Remove the 2nd acl added to the list. - boolean remove = store.removeAcl(ozoneObj, acls.get(1)); - assertTrue(remove); - assertThat(store.getAcl(ozoneObj)).doesNotContain(acls.get(1)); + assertTrue(store.removeAcl(ozoneObj, acl2)); + assertThat(store.getAcl(ozoneObj)).doesNotContain(acl2); - remove = store.removeAcl(ozoneObj, acls.get(0)); - assertTrue(remove); - assertThat(store.getAcl(ozoneObj)).doesNotContain(acls.get(0)); + assertTrue(store.removeAcl(ozoneObj, userAcl)); + assertThat(store.getAcl(ozoneObj)).doesNotContain(userAcl); } @Test