sodonnel commented on a change in pull request #3158:
URL: https://github.com/apache/ozone/pull/3158#discussion_r819506721
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelinePlacementPolicy.java
##########
@@ -84,40 +85,28 @@ public PipelinePlacementPolicy(final NodeManager
nodeManager,
this.heavyNodeCriteria = dnLimit == null ? 0 : Integer.parseInt(dnLimit);
}
- int currentPipelineCount(DatanodeDetails datanodeDetails, int nodesRequired)
{
-
- // Datanodes from pipeline in some states can also be considered available
- // for pipeline allocation. Thus the number of these pipeline shall be
- // deducted from total heaviness calculation.
- int pipelineNumDeductable = 0;
- Set<PipelineID> pipelines = nodeManager.getPipelines(datanodeDetails);
- for (PipelineID pid : pipelines) {
- Pipeline pipeline;
- try {
- pipeline = stateManager.getPipeline(pid);
- } catch (PipelineNotFoundException e) {
- LOG.debug("Pipeline not found in pipeline state manager during" +
- " pipeline creation. PipelineID: {}", pid, e);
- continue;
- }
- if (pipeline != null &&
- // single node pipeline are not accounted for while determining
- // the pipeline limit for dn
- pipeline.getType() == HddsProtos.ReplicationType.RATIS &&
- (RatisReplicationConfig
- .hasFactor(pipeline.getReplicationConfig(),
ReplicationFactor.ONE)
- ||
- pipeline.getReplicationConfig().getRequiredNodes()
- == nodesRequired &&
- pipeline.getPipelineState()
- == Pipeline.PipelineState.CLOSED)) {
- pipelineNumDeductable++;
- }
- }
- return pipelines.size() - pipelineNumDeductable;
+ int currentRatisThreePipelineCount(DatanodeDetails datanodeDetails) {
+ // Safe to cast collection's size to int
+ return (int) nodeManager.getPipelines(datanodeDetails).stream()
+ .map(id -> {
+ try {
+ return stateManager.getPipeline(id);
+ } catch (PipelineNotFoundException e) {
+ LOG.debug("Pipeline not found in pipeline state manager during" +
+ " pipeline creation. PipelineID: {}", id, e);
+ return null;
+ }
+ })
+ .filter(this::isNonClosedRatisThreePipeline)
+ .count();
}
-
+ private boolean isNonClosedRatisThreePipeline(Pipeline p) {
+ return p.getType() == RATIS
Review comment:
Yea, I was just looking at the code to see where else we could benefit
from this. I think it would be a good idea to a constant in
RatisReplicationConfig for ONE and THREE. We could have a separate Jira to move
any existing calls to `new RatisReplicationConfig...` over to the constant in
non-test code, which might save some allocations.
I wonder if we would be better with a static method in
RatisReplicationConfig, perhaps `getInstance(Factor factor)` - right now every
ContainerInfo object in SCM holds a unique ReplicationConfig object - if we
change the code correctly we could reduce all those duplicated instances down
to 1. The object only holds a single field, but it will still make a small
difference to memory.
I will create a followup Jira for this purpose and then we could implement
that part separately to avoid blocking this one.
--
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]
