adoroszlai commented on code in PR #8216:
URL: https://github.com/apache/ozone/pull/8216#discussion_r2051010859
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/RatisPipelineProvider.java:
##########
@@ -147,10 +156,15 @@ public synchronized Pipeline
create(RatisReplicationConfig replicationConfig,
List<DatanodeDetails> excludedNodes, List<DatanodeDetails> favoredNodes)
throws IOException {
if (exceedPipelineNumberLimit(replicationConfig)) {
- throw new SCMException("Ratis pipeline number meets the limit: " +
- pipelineNumberLimit + " replicationConfig : " +
- replicationConfig,
- SCMException.ResultCodes.FAILED_TO_FIND_SUITABLE_NODE);
+ String limitInfo = (maxPipelinePerDatanode > 0)
+ ? String.format("per datanode: %d", maxPipelinePerDatanode)
+ : String.format(": %d", pipelineNumberLimit);
+
+ throw new SCMException(
+ String.format("Ratis pipeline number meets the limit %s
replicationConfig: %s",
Review Comment:
I think "Cannot create pipeline as it would exceed the limit" would be
easier to understand than "Ratis pipeline number meets the limit".
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java:
##########
@@ -465,6 +465,18 @@ private void assertPipelineCreationFailsWithNotEnoughNodes(
actualNodeCount);
}
+ private void assertPipelineCreationFailsWithExceedingLimit(int limit) {
+ SCMException ex = assertThrows(SCMException.class, () -> {
+ ReplicationConfig ratisThree =
+ ReplicationConfig.fromProtoTypeAndFactor(
+ HddsProtos.ReplicationType.RATIS,
+ HddsProtos.ReplicationFactor.THREE);
+ scm.getPipelineManager().createPipeline(ratisThree);
+ }, "3 nodes should not have been found for a pipeline.");
Review Comment:
nit: Get replication config outside of `assertThrows`, then block lambda can
be simplified to statement.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/RatisPipelineProvider.java:
##########
@@ -101,32 +101,41 @@ public RatisPipelineProvider(NodeManager nodeManager,
}
}
- private boolean exceedPipelineNumberLimit(
- RatisReplicationConfig replicationConfig) {
+ private boolean exceedPipelineNumberLimit(RatisReplicationConfig
replicationConfig) {
+ // Apply limits only for replication factor THREE
if (replicationConfig.getReplicationFactor() != ReplicationFactor.THREE) {
- // Only put limits for Factor THREE pipelines.
return false;
}
- // Per datanode limit
+
+ PipelineStateManager pipelineStateManager = getPipelineStateManager();
+
+ int totalActivePipelines =
pipelineStateManager.getPipelines(replicationConfig).size();
+
+ int closedPipelines = pipelineStateManager.getPipelines(replicationConfig,
PipelineState.CLOSED).size();
+
+ int openPipelines = totalActivePipelines - closedPipelines;
Review Comment:
Empty line before/after blocks (`if`, `try-catch`, etc.) is fine, but
unnecessary between a list of statements. Please avoid this.
--
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]
