ivandika3 commented on code in PR #10983:
URL: https://github.com/apache/ozone/pull/10983#discussion_r3793455906
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java:
##########
Review Comment:
All these storage space logic should be put to
`SCMNodeManager#hasAvailableSpace`, otherwise we have two conflicting space
accounting logic. We can address it in HDDS-16045.
cc: @F64116045
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/ECPipelineProvider.java:
##########
@@ -141,19 +146,13 @@ public Pipeline createForRead(
dns.sort(Comparator.comparing(nodeStatusMap::get,
CREATE_FOR_READ_COMPARATOR));
- // Read Pipelines do not require storage tiers, so the calculation of
storage tiers can be omitted.
- return createPipelineInternal(replicationConfig, dns, map, null);
- }
-
- private Pipeline createPipelineInternal(ECReplicationConfig repConfig,
- List<DatanodeDetails> dns, Map<DatanodeDetails, Integer> indexes,
StorageTier storageTier) {
- return Pipeline.newBuilder()
- .setId(PipelineID.randomId())
- .setState(Pipeline.PipelineState.ALLOCATED)
- .setReplicationConfig(repConfig)
- .setNodes(dns)
- .setReplicaIndexes(indexes)
- .setSupportedStorageTier(storageTier)
+ // Use insecureRandomId for throwaway read pipeline IDs to avoid
+ // contention on the shared SecureRandom instance.
+ // Read Pipelines do not require storage tiers, so no supported tier is
set.
+ return newPipelineBuilder(replicationConfig, dns)
+ .setId(PipelineID.insecureRandomId())
+ .setReplicaIndexes(map)
+ .setSupportedStorageTier(null)
Review Comment:
Keep the comment
`// Read Pipelines do not require storage tiers, so the calculation of
storage tiers can be omitted.`
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/SimplePipelineProvider.java:
##########
@@ -117,16 +107,23 @@ public Pipeline create(StandaloneReplicationConfig
replicationConfig,
@Override
public Pipeline createForRead(StandaloneReplicationConfig replicationConfig,
Set<ContainerReplica> replicas) {
- // Read Pipelines do not require storage tiers, so the calculation of
storage tiers can be omitted.
- return createPipelineInternal(replicationConfig, replicas
- .stream()
- .map(ContainerReplica::getDatanodeDetails)
- .collect(Collectors.toList()), null);
+ // Use insecureRandomId for throwaway read pipeline IDs to avoid
+ // contention on the shared SecureRandom instance.
+ // Read Pipelines do not require storage tiers, so no supported tier is
set.
+ return newPipelineBuilder(replicationConfig,
ContainerReplica.toDatanodeDetailsList(replicas))
+ .setId(PipelineID.insecureRandomId())
+ .setSupportedStorageTier(null)
Review Comment:
Nit: Also keep the comment
`// Read Pipelines do not require storage tiers, so the calculation of
storage tiers can be omitted.`
--
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]
