HDDS-15250. PipelineProvider support create pipeline by StorageTier#10410
HDDS-15250. PipelineProvider support create pipeline by StorageTier#10410xichen01 wants to merge 1 commit into
Conversation
|
@greenwich @chungen0126 @ivandika3 @peterxcli Please help to review |
| public static void setDefault(StorageTier storageTier) { | ||
| defaultTier = storageTier; | ||
| } |
There was a problem hiding this comment.
Does not seem to be used, should be configured and set in StorageContainerManager.java
| if (storageTier.equals(StorageTier.EMPTY)) { | ||
| throw new SCMException("Cannot create Pipeline for empty tier", | ||
| SCMException.ResultCodes.CANNOT_CREATE_PIPELINE_FOR_EMPTY_TIER); | ||
| } |
| protected abstract Pipeline create(REPLICATION_CONFIG replicationConfig) | ||
| throws IOException; | ||
| protected abstract Pipeline create(REPLICATION_CONFIG replicationConfig, | ||
| StorageTier storageTier) throws IOException; |
There was a problem hiding this comment.
Could StorageTier be part of REPLICATION_CONFIG so we do not need to change abstract interface here?
| private final boolean isUniform; | ||
| private static final Map<StorageTier, Map<Integer, List<StorageType>>> | ||
| CACHE = new EnumMap<>(StorageTier.class); | ||
| public static final int MAX_NODE_COUNT = 20; |
There was a problem hiding this comment.
MAX_NODE_COUNT = 20 is undocumented and, because getStorageTypes(int) throws on a cache miss, it effectively silently caps replication width.
For uniform tiers, computeStorageTypes is just Collections.nCopies(...) (O(1)), so the cache isn't buying anything — suggest dropping it:
public List<StorageType> getStorageTypes(int nodeCount) {
return computeStorageTypes(nodeCount);
}
I checked that later fromID/computeId/findSupportedStorageTiers work — none of it needs this cache (only the StorageType→prime table + computeId), so dropping it entirely is safe.
(Or, if you'd rather keep the precompute: fall through to computeStorageTypes(nodeCount) on a miss, document MAX_NODE_COUNT, and make it private.)
| * @param defaultContainerSize The cluster default max container size | ||
| * @param container The container to select new replicas for | ||
| * @param container The container to select new replicas for | ||
| * @param storageType |
There was a problem hiding this comment.
nit: empty storageType param
4 participants
What changes were proposed in this pull request?
FYI:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15250
How was this patch tested?
Newly added test