hemantk-12 commented on code in PR #4042: URL: https://github.com/apache/ozone/pull/4042#discussion_r1042653693
########## hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/BucketObjectDBInfo.java: ########## @@ -0,0 +1,202 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.recon.api.types; + +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.hadoop.hdds.client.DefaultReplicationConfig; +import org.apache.hadoop.hdds.protocol.StorageType; +import org.apache.hadoop.ozone.om.helpers.BucketEncryptionKeyInfo; +import org.apache.hadoop.ozone.om.helpers.BucketLayout; +import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; + +/** + * Encapsulates the low level bucket info. + */ +public class BucketObjectDBInfo extends ObjectDBInfo { + @JsonProperty("volumeName") + private String volumeName; + + @JsonProperty("storageType") + private StorageType storageType; + + @JsonProperty("versioning") + private boolean isVersioningEnabled; + + @JsonProperty("usedBytes") + private String usedBytes; + + @JsonProperty("encryptionInfo") + private BucketEncryptionKeyInfo bekInfo; + + @JsonProperty("replicationConfigInfo") + private DefaultReplicationConfig defaultReplicationConfig; + + @JsonProperty("sourceVolume") + private String sourceVolume; + + @JsonProperty("sourceBucket") + private String sourceBucket; + + @JsonProperty("bucketLayout") + private BucketLayout bucketLayout; + + @JsonProperty("owner") + private String owner; + + public static BucketObjectDBInfo.Builder newBuilder() { + return new BucketObjectDBInfo.Builder(); + } + + public BucketObjectDBInfo() { + + } + + public BucketObjectDBInfo(Builder b) { Review Comment: This is not how builder pattern works. Please change all the builder to this way. ```suggestion public BucketObjectDBInfo(Map<String, String> metadata, String name, String volumeName, StorageType storageType, boolean isVersioningEnabled, BucketEncryptionKeyInfo bekInfo, DefaultReplicationConfig defaultReplicationConfig, String sourceVolume, String sourceBucket, BucketLayout bucketLayout, String owner, long quotaInBytes, long quotaInNamespace, long usedNamespace, long creationTime, long modificationTime, List<OzoneAcl> acls) { super.setMetadata(metadata); super.setName(name); super.setQuotaInBytes(quotaInBytes); super.setQuotaInNamespace(quotaInNamespace); super.setUsedNamespace(usedNamespace); super.setCreationTime(creationTime); super.setModificationTime(modificationTime); super.setAcls(acls); this.volumeName = volumeName; this.sourceBucket = sourceVolume; this.sourceVolume = sourceBucket; this.isVersioningEnabled = isVersioningEnabled; this.storageType = storageType; this.defaultReplicationConfig = defaultReplicationConfig; this.bucketLayout = bucketLayout; this.owner = owner; this.bekInfo = bekInfo; } ``` More: https://www.baeldung.com/java-builder-pattern-freebuilder ########## hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/BucketObjectDBInfo.java: ########## @@ -0,0 +1,202 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.recon.api.types; + +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.hadoop.hdds.client.DefaultReplicationConfig; +import org.apache.hadoop.hdds.protocol.StorageType; +import org.apache.hadoop.ozone.om.helpers.BucketEncryptionKeyInfo; +import org.apache.hadoop.ozone.om.helpers.BucketLayout; +import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; + +/** + * Encapsulates the low level bucket info. + */ +public class BucketObjectDBInfo extends ObjectDBInfo { + @JsonProperty("volumeName") + private String volumeName; + + @JsonProperty("storageType") + private StorageType storageType; + + @JsonProperty("versioning") + private boolean isVersioningEnabled; + + @JsonProperty("usedBytes") + private String usedBytes; + + @JsonProperty("encryptionInfo") + private BucketEncryptionKeyInfo bekInfo; + + @JsonProperty("replicationConfigInfo") + private DefaultReplicationConfig defaultReplicationConfig; + + @JsonProperty("sourceVolume") + private String sourceVolume; + + @JsonProperty("sourceBucket") + private String sourceBucket; + + @JsonProperty("bucketLayout") + private BucketLayout bucketLayout; + + @JsonProperty("owner") + private String owner; + + public static BucketObjectDBInfo.Builder newBuilder() { + return new BucketObjectDBInfo.Builder(); + } + + public BucketObjectDBInfo() { + + } + + public BucketObjectDBInfo(Builder b) { + this.setMetadata(b.getOmBucketInfo().getMetadata()); + this.setVolumeName(b.getOmBucketInfo().getVolumeName()); + this.setName(b.getOmBucketInfo().getBucketName()); + this.setQuotaInBytes(b.getOmBucketInfo().getQuotaInBytes()); + this.setQuotaInNamespace( + b.getOmBucketInfo().getQuotaInNamespace()); + this.setUsedNamespace(b.getOmBucketInfo().getUsedNamespace()); + this.setCreationTime(b.getOmBucketInfo().getCreationTime()); + this.setModificationTime( + b.getOmBucketInfo().getModificationTime()); + this.setAcls(b.getOmBucketInfo().getAcls()); + this.setSourceBucket(b.getOmBucketInfo().getSourceBucket()); + this.setSourceVolume(b.getOmBucketInfo().getSourceVolume()); + this.setBekInfo(b.getOmBucketInfo().getEncryptionKeyInfo()); + this.setVersioningEnabled( + b.getOmBucketInfo().getIsVersionEnabled()); + this.setStorageType(b.getOmBucketInfo().getStorageType()); + this.setDefaultReplicationConfig( + b.getOmBucketInfo().getDefaultReplicationConfig()); + this.setBucketLayout(b.getOmBucketInfo().getBucketLayout()); + this.setOwner(b.getOmBucketInfo().getOwner()); + } + + public String getVolumeName() { + return volumeName; + } + + public void setVolumeName(String volumeName) { + this.volumeName = volumeName; + } + + public StorageType getStorageType() { + return storageType; + } + + public void setStorageType(StorageType storageType) { + this.storageType = storageType; + } + + public String getUsedBytes() { + return usedBytes; + } + + public void setUsedBytes(String usedBytes) { + this.usedBytes = usedBytes; + } + + public BucketEncryptionKeyInfo getBekInfo() { + return bekInfo; + } + + public void setBekInfo(BucketEncryptionKeyInfo bekInfo) { + this.bekInfo = bekInfo; + } + + public DefaultReplicationConfig getDefaultReplicationConfig() { + return defaultReplicationConfig; + } + + public void setDefaultReplicationConfig( + DefaultReplicationConfig defaultReplicationConfig) { + this.defaultReplicationConfig = defaultReplicationConfig; + } + + public String getSourceVolume() { + return sourceVolume; + } + + public void setSourceVolume(String sourceVolume) { + this.sourceVolume = sourceVolume; + } + + public String getSourceBucket() { + return sourceBucket; + } + + public void setSourceBucket(String sourceBucket) { + this.sourceBucket = sourceBucket; + } + + public boolean isVersioningEnabled() { + return isVersioningEnabled; + } + + public void setVersioningEnabled(boolean versioningEnabled) { + isVersioningEnabled = versioningEnabled; + } + + public BucketLayout getBucketLayout() { + return bucketLayout; + } + + public void setBucketLayout(BucketLayout bucketLayout) { + this.bucketLayout = bucketLayout; + } + + public String getOwner() { + return owner; + } + + public void setOwner(String owner) { + this.owner = owner; + } + + /** + * Builder for BucketObjectDBInfo. + */ + @SuppressWarnings("checkstyle:hiddenfield") + public static final class Builder { Review Comment: ```suggestion public static class Builder { private final OmBucketInfo omBucketInfo; public Builder(OmBucketInfo omBucketInfo) { this.omBucketInfo = omBucketInfo; } public BucketObjectDBInfo build() { if (this.omBucketInfo == null) { return new BucketObjectDBInfo(); } return new BucketObjectDBInfo(omBucketInfo.getMetadata(), omBucketInfo.getBucketName(), omBucketInfo.getVolumeName(), omBucketInfo.getStorageType(), omBucketInfo.getIsVersionEnabled(), omBucketInfo.getEncryptionKeyInfo(), omBucketInfo.getDefaultReplicationConfig(), omBucketInfo.getSourceVolume(), omBucketInfo.getSourceBucket(), omBucketInfo.getBucketLayout(), omBucketInfo.getOwner(), omBucketInfo.getQuotaInBytes(), omBucketInfo.getQuotaInNamespace(), omBucketInfo.getUsedNamespace(), omBucketInfo.getCreationTime(), omBucketInfo.getModificationTime(), omBucketInfo.getAcls()); } } ``` ########## hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/BucketObjectDBInfo.java: ########## @@ -0,0 +1,202 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.recon.api.types; + +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.hadoop.hdds.client.DefaultReplicationConfig; +import org.apache.hadoop.hdds.protocol.StorageType; +import org.apache.hadoop.ozone.om.helpers.BucketEncryptionKeyInfo; +import org.apache.hadoop.ozone.om.helpers.BucketLayout; +import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; + +/** + * Encapsulates the low level bucket info. + */ +public class BucketObjectDBInfo extends ObjectDBInfo { Review Comment: Is it really a subclass of `ObjectDBInfo`? Or you just did it because they share some common properties? It also doesn't follow the correct hierarchy model. Java doc comment says `VolumeObjectDBInfo` encapsulates the low level volume info, `BucketObjectDBInfo` encapsulates the low level bucket info and `ObjectDBInfo`Encapsulates the low level volume/bucket/dir info. So hierarchy should be Volume -> Bucket -> Dir -> Key but according to your implementation Dir -> Volume, Bucket and Key. Which is wrong to me. Please correct it. If you want to extract out common properties, create another class something `CommonPropertyClass` (Please come up with better name). ``` VolumeObjectDBInfo extands CommonPropertyClass BucketObjectDBInfo extands CommonPropertyClass ObjectDBInfo extands CommonPropertyClass KeyObjectDBInfo extands CommonPropertyClass ``` ########## hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/BucketObjectDBInfo.java: ########## @@ -0,0 +1,202 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.recon.api.types; + +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.hadoop.hdds.client.DefaultReplicationConfig; +import org.apache.hadoop.hdds.protocol.StorageType; +import org.apache.hadoop.ozone.om.helpers.BucketEncryptionKeyInfo; +import org.apache.hadoop.ozone.om.helpers.BucketLayout; +import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; + +/** + * Encapsulates the low level bucket info. + */ +public class BucketObjectDBInfo extends ObjectDBInfo { + @JsonProperty("volumeName") + private String volumeName; + + @JsonProperty("storageType") + private StorageType storageType; + + @JsonProperty("versioning") + private boolean isVersioningEnabled; + + @JsonProperty("usedBytes") + private String usedBytes; + + @JsonProperty("encryptionInfo") + private BucketEncryptionKeyInfo bekInfo; + + @JsonProperty("replicationConfigInfo") + private DefaultReplicationConfig defaultReplicationConfig; + + @JsonProperty("sourceVolume") + private String sourceVolume; + + @JsonProperty("sourceBucket") + private String sourceBucket; + + @JsonProperty("bucketLayout") + private BucketLayout bucketLayout; + + @JsonProperty("owner") + private String owner; + + public static BucketObjectDBInfo.Builder newBuilder() { + return new BucketObjectDBInfo.Builder(); + } + + public BucketObjectDBInfo() { + + } + + public BucketObjectDBInfo(Builder b) { + this.setMetadata(b.getOmBucketInfo().getMetadata()); + this.setVolumeName(b.getOmBucketInfo().getVolumeName()); + this.setName(b.getOmBucketInfo().getBucketName()); + this.setQuotaInBytes(b.getOmBucketInfo().getQuotaInBytes()); + this.setQuotaInNamespace( + b.getOmBucketInfo().getQuotaInNamespace()); + this.setUsedNamespace(b.getOmBucketInfo().getUsedNamespace()); + this.setCreationTime(b.getOmBucketInfo().getCreationTime()); + this.setModificationTime( + b.getOmBucketInfo().getModificationTime()); + this.setAcls(b.getOmBucketInfo().getAcls()); + this.setSourceBucket(b.getOmBucketInfo().getSourceBucket()); + this.setSourceVolume(b.getOmBucketInfo().getSourceVolume()); + this.setBekInfo(b.getOmBucketInfo().getEncryptionKeyInfo()); + this.setVersioningEnabled( + b.getOmBucketInfo().getIsVersionEnabled()); + this.setStorageType(b.getOmBucketInfo().getStorageType()); + this.setDefaultReplicationConfig( + b.getOmBucketInfo().getDefaultReplicationConfig()); + this.setBucketLayout(b.getOmBucketInfo().getBucketLayout()); + this.setOwner(b.getOmBucketInfo().getOwner()); + } + + public String getVolumeName() { + return volumeName; + } + + public void setVolumeName(String volumeName) { Review Comment: Why do we need setter? Is it for jackson serialization? ########## hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/NSSummaryEndpoint.java: ########## @@ -80,7 +80,8 @@ public Response getBasicInfo( NamespaceSummaryResponse namespaceSummaryResponse; if (!isInitializationComplete()) { namespaceSummaryResponse = - new NamespaceSummaryResponse(EntityType.UNKNOWN); + NamespaceSummaryResponse.newBuilder(). Review Comment: ```suggestion NamespaceSummaryResponse.newBuilder() .setEntityType(EntityType.UNKNOWN) .setStatus(ResponseStatus.INITIALIZING) .build(); ``` ########## hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/ObjectDBInfo.java: ########## @@ -0,0 +1,210 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.recon.api.types; + +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.hadoop.ozone.OzoneAcl; +import org.apache.hadoop.ozone.om.helpers.OmDirectoryInfo; +import org.apache.hadoop.ozone.om.helpers.OmPrefixInfo; + +import java.util.List; +import java.util.Map; + +/** + * Encapsulates the low level volume/bucket/dir info. + */ +public class ObjectDBInfo { Review Comment: Should we just call it `ObjectInfo`? Why DB in the name? ########## hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/handlers/BucketEntityHandler.java: ########## @@ -49,16 +52,34 @@ public BucketEntityHandler( @Override public NamespaceSummaryResponse getSummaryResponse() throws IOException { - NamespaceSummaryResponse namespaceSummaryResponse = - new NamespaceSummaryResponse(EntityType.BUCKET); + String[] names = getNames(); assert (names.length == 2); long bucketObjectId = getBucketHandler().getBucketObjectId(names); - namespaceSummaryResponse - .setNumTotalDir(getTotalDirCount(bucketObjectId)); - namespaceSummaryResponse.setNumTotalKey(getTotalKeyCount(bucketObjectId)); - return namespaceSummaryResponse; + CountStats countStats = new CountStats( + -1, -1, + getTotalDirCount(bucketObjectId), getTotalKeyCount(bucketObjectId)); + return + NamespaceSummaryResponse.newBuilder() + .setEntityType(EntityType.BUCKET) + .setCountStats(countStats) + .setObjectDBInfo(getBucketObjDbInfo(names)) + .setStatus(ResponseStatus.OK) + .build(); Review Comment: It could be in one line. Please change it everywhere. ```suggestion return NamespaceSummaryResponse.newBuilder() .setEntityType(EntityType.VOLUME) .setCountStats(countStats) .setObjectDBInfo(getVolumeObjDbInfo(names)) .setStatus(ResponseStatus.OK) .build(); ``` ########## hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/BucketObjectDBInfo.java: ########## @@ -0,0 +1,202 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.recon.api.types; + +import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.hadoop.hdds.client.DefaultReplicationConfig; +import org.apache.hadoop.hdds.protocol.StorageType; +import org.apache.hadoop.ozone.om.helpers.BucketEncryptionKeyInfo; +import org.apache.hadoop.ozone.om.helpers.BucketLayout; +import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; + +/** + * Encapsulates the low level bucket info. + */ +public class BucketObjectDBInfo extends ObjectDBInfo { + @JsonProperty("volumeName") + private String volumeName; + + @JsonProperty("storageType") + private StorageType storageType; + + @JsonProperty("versioning") + private boolean isVersioningEnabled; + + @JsonProperty("usedBytes") + private String usedBytes; + + @JsonProperty("encryptionInfo") + private BucketEncryptionKeyInfo bekInfo; + + @JsonProperty("replicationConfigInfo") + private DefaultReplicationConfig defaultReplicationConfig; + + @JsonProperty("sourceVolume") + private String sourceVolume; + + @JsonProperty("sourceBucket") + private String sourceBucket; + + @JsonProperty("bucketLayout") + private BucketLayout bucketLayout; + + @JsonProperty("owner") + private String owner; + + public static BucketObjectDBInfo.Builder newBuilder() { Review Comment: This is unnecessary function. ########## hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestNSSummaryEndpointWithLegacy.java: ########## @@ -389,15 +399,47 @@ public void testUtility() { @Test public void testGetBasicInfoRoot() throws Exception { + String username = "myuser"; + OmPrefixInfo omPrefixInfo = getOmPrefixInfoForTest(ROOT_PATH, + IAccessAuthorizer.ACLIdentityType.USER, + username, + IAccessAuthorizer.ACLType.WRITE, + ACCESS); + omPrefixInfo.getMetadata().put("key", "value"); + reconOMMetadataManager.getPrefixTable() + .put(OzoneConsts.OM_KEY_PREFIX, omPrefixInfo); // Test root basics Response rootResponse = nsSummaryEndpoint.getBasicInfo(ROOT_PATH); NamespaceSummaryResponse rootResponseObj = (NamespaceSummaryResponse) rootResponse.getEntity(); Assert.assertEquals(EntityType.ROOT, rootResponseObj.getEntityType()); - Assert.assertEquals(2, rootResponseObj.getNumVolume()); - Assert.assertEquals(4, rootResponseObj.getNumBucket()); - Assert.assertEquals(5, rootResponseObj.getNumTotalDir()); - Assert.assertEquals(10, rootResponseObj.getNumTotalKey()); + Assert.assertEquals(2, rootResponseObj.getCountStats().getNumVolume()); + Assert.assertEquals(4, rootResponseObj.getCountStats().getNumBucket()); + Assert.assertEquals(5, rootResponseObj.getCountStats().getNumTotalDir()); + Assert.assertEquals(10, rootResponseObj.getCountStats().getNumTotalKey()); Review Comment: I don't think this is addressed. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
