chungen0126 commented on code in PR #10191: URL: https://github.com/apache/ozone/pull/10191#discussion_r3189885224
########## hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneStoragePolicy.java: ########## @@ -0,0 +1,104 @@ +/* + * 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 + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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.hdds.client; + +import org.apache.hadoop.hdds.protocol.proto.HddsProtos.StoragePolicyProto; + +/** + * Enum defining different storage policies using StorageTier. + */ +public enum OzoneStoragePolicy implements StoragePolicy { + + HOT("Hot", StorageTier.SSD, StorageTier.DISK), + WARM("Warm", StorageTier.DISK, StorageTier.EMPTY), + COLD("Cold", StorageTier.ARCHIVE, StorageTier.EMPTY); + + private final String name; + private final StorageTier creationTier; + private final StorageTier creationFallbackTier; + + OzoneStoragePolicy(String name, StorageTier creationTier, + StorageTier creationFallbackTier) { + this.name = name; + this.creationTier = creationTier; + this.creationFallbackTier = creationFallbackTier; + } + + @Override + public String getName() { + return name; + } + + @Override + public StorageTier getCreationTier() { + return creationTier; + } + + @Override + public StorageTier getCreationFallbackTier() { + return creationFallbackTier; + } + + /** + * Converts the current StoragePolicyType to its protobuf representation. + * @return the corresponding StoragePolicyProto. + */ + public StoragePolicyProto toProto() { + switch (this) { + case HOT: + return StoragePolicyProto.HOT; + case WARM: + return StoragePolicyProto.WARM; + case COLD: + return StoragePolicyProto.COLD; + default: + throw new IllegalArgumentException( + "Error: StoragePolicyType not found, type=" + this); Review Comment: nit: The class name is `OzoneStoragePolicy`. ```suggestion "Error: OzoneStoragePolicy not found, type=" + this); ``` ########## hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/StorageTier.java: ########## @@ -0,0 +1,170 @@ +/* + * 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 + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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.hdds.client; + +import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.ONE; +import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.THREE; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.EnumMap; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.hadoop.fs.StorageType; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos.StorageTierProto; + +/** + * Ozone specific storage tiers. + */ +public enum StorageTier { + SSD("SSD", StorageType.SSD), + DISK("DISK", StorageType.DISK), + ARCHIVE("ARCHIVE", StorageType.ARCHIVE), + EMPTY("EMPTY"); + + private final String tierName; + private final List<StorageType> storageTypes; + private final boolean uniformStorageType; + private static final Map<StorageTier, Map<ReplicationConfig, List<StorageType>>> + CACHE = new EnumMap<>(StorageTier.class); + + StorageTier(String tierName) { + this.tierName = tierName; + this.storageTypes = Collections.emptyList(); + this.uniformStorageType = true; + } + + // Constructor for uniform storage tiers + StorageTier(String tierName, StorageType uniformStorageType) { + this.tierName = tierName; + this.storageTypes = Collections.singletonList(uniformStorageType); + this.uniformStorageType = true; + } + + // Constructor for non-uniform storage tiers + StorageTier(String tierName, StorageType... storageTypes) { + this.tierName = tierName; + if (Arrays.stream(storageTypes).distinct().count() <= 1) { + throw new IllegalArgumentException("StorageTier '" + tierName + + "' requires at least two different StorageType instances." + + " but only " + Arrays.stream(storageTypes).distinct().count() + + " StorageType were provided."); + } + this.storageTypes = Arrays.asList(storageTypes); + this.uniformStorageType = false; + } + + static { + // Precompute storage type mappings for each replication config + for (StorageTier tier : StorageTier.values()) { + Map<ReplicationConfig, List<StorageType>> tierCache = new HashMap<>(); + List<ReplicationConfig> replicationConfigs = Arrays.asList( + RatisReplicationConfig.getInstance(ONE), + RatisReplicationConfig.getInstance(THREE), + StandaloneReplicationConfig.getInstance(ONE), + StandaloneReplicationConfig.getInstance(THREE) + ); + + for (ReplicationConfig config : replicationConfigs) { + tierCache.put(config, tier.computeStorageTypes(config)); + } + CACHE.put(tier, tierCache); + } + } + + public StorageTierProto toProto() { + switch (this) { + case SSD: + return StorageTierProto.SSD_TIER; + case DISK: + return StorageTierProto.DISK_TIER; + case ARCHIVE: + return StorageTierProto.ARCHIVE_TIER; + default: + throw new IllegalStateException( + "Illegal StorageTier: " + this); + } + } + + public static StorageTier fromProto(StorageTierProto tier) { + switch (tier) { + case SSD_TIER: + return SSD; + case DISK_TIER: + return DISK; + case ARCHIVE_TIER: + return ARCHIVE; + default: + throw new IllegalStateException( + "Illegal StorageTierProto: " + tier); + } + } + + public String getTierName() { + return tierName; + } + + public boolean isUniformStorageType() { + return uniformStorageType; + } + + /** + * Computes the list of StorageTypes based on replication configuration. + * + * @param replicationConfig The replication configuration. + * @return The list of StorageTypes for the given tier and replication configuration. + */ + private List<StorageType> computeStorageTypes( + ReplicationConfig replicationConfig) { + if (isUniformStorageType()) { + int numberOfNodes = replicationConfig.getRequiredNodes(); + if (storageTypes.isEmpty()) { + return Collections.emptyList(); + } + return new ArrayList<>(Collections.nCopies(numberOfNodes, storageTypes.get(0))); Review Comment: Should we consider making the `StorageType` list read-only to ensure immutability? ```suggestion return Collections.unmodifiableList(new ArrayList<>(Collections.nCopies(numberOfNodes, storageTypes.get(0))); ``` ########## hadoop-ozone/common/src/test/java/org/apache/hadoop/hdds/protocol/StoragePolicyTest.java: ########## @@ -0,0 +1,48 @@ +/* + * 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 + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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.hdds.protocol; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.apache.hadoop.hdds.client.OzoneStoragePolicy; +import org.apache.hadoop.hdds.client.StorageTier; +import org.junit.jupiter.api.Test; + +class StoragePolicyTest { Review Comment: Could you please add some unit tests to cover the Protobuf/Java conversion? ########## hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/StorageTier.java: ########## @@ -0,0 +1,170 @@ +/* + * 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 + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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.hdds.client; + +import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.ONE; +import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.THREE; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.EnumMap; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.hadoop.fs.StorageType; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos.StorageTierProto; + +/** + * Ozone specific storage tiers. + */ +public enum StorageTier { + SSD("SSD", StorageType.SSD), + DISK("DISK", StorageType.DISK), + ARCHIVE("ARCHIVE", StorageType.ARCHIVE), + EMPTY("EMPTY"); + + private final String tierName; + private final List<StorageType> storageTypes; + private final boolean uniformStorageType; + private static final Map<StorageTier, Map<ReplicationConfig, List<StorageType>>> + CACHE = new EnumMap<>(StorageTier.class); + + StorageTier(String tierName) { + this.tierName = tierName; + this.storageTypes = Collections.emptyList(); + this.uniformStorageType = true; + } + + // Constructor for uniform storage tiers + StorageTier(String tierName, StorageType uniformStorageType) { + this.tierName = tierName; + this.storageTypes = Collections.singletonList(uniformStorageType); + this.uniformStorageType = true; + } + + // Constructor for non-uniform storage tiers + StorageTier(String tierName, StorageType... storageTypes) { + this.tierName = tierName; + if (Arrays.stream(storageTypes).distinct().count() <= 1) { + throw new IllegalArgumentException("StorageTier '" + tierName + + "' requires at least two different StorageType instances." + + " but only " + Arrays.stream(storageTypes).distinct().count() + + " StorageType were provided."); + } Review Comment: While I understand that non-uniform storage tiers aren't supported yet (so this line might not be reachable), `Arrays.stream(storageTypes).distinct().count()` is called twice here. We could assign it to a variable instead. ```suggestion long distinctStorageTypes = Arrays.stream(storageTypes).distinct().count(); if (distinctStorageTypes <= 1) { throw new IllegalArgumentException("StorageTier '" + tierName + "' requires at least two different StorageType instances." + " but only " + distinctStorageTypes + " StorageType were provided."); } ``` -- 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]
