From df1295abdb91b91e3e1ae515e965cfa83123ca47 Mon Sep 17 00:00:00 2001 From: Uma Maheswara Rao G Date: Tue, 24 May 2022 20:46:14 -0700 Subject: [PATCH 1/4] HDDS-6795: EC: PipelineStateMap#addPipeline should not have precondition checks post db updates --- .../hdds/scm/pipeline/PipelineManagerImpl.java | 12 ++++++++++++ .../hadoop/hdds/scm/pipeline/PipelineStateMap.java | 8 -------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java index 4cb96b1d2fd4..72dd7c272f70 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java @@ -214,6 +214,18 @@ public Pipeline createPipeline(ReplicationConfig replicationConfig, try { Pipeline pipeline = pipelineFactory.create(replicationConfig, excludedNodes, favoredNodes); + // In case in case if provided pipeline provider returns null. + if (pipeline == null) { + throw new IOException("Pipeline cannot be null"); + } + // In case if provided pipeline returns less number of nodes than + // required. + if (pipeline.getNodes().size() != pipeline.getReplicationConfig() + .getRequiredNodes()) { + throw new IOException("Nodes size= " + pipeline.getNodes() + .size() + ", replication factor=" + pipeline.getReplicationConfig() + .getRequiredNodes() + " do not match"); + } stateManager.addPipeline(pipeline.getProtobufMessage( ClientVersion.CURRENT_VERSION)); recordMetricsForPipeline(pipeline); diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java index 6b40f28fc0b1..e61d50296496 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java @@ -70,14 +70,6 @@ class PipelineStateMap { * @throws IOException if pipeline with provided pipelineID already exists */ void addPipeline(Pipeline pipeline) throws IOException { - Preconditions.checkNotNull(pipeline, "Pipeline cannot be null"); - Preconditions.checkArgument( - pipeline.getNodes().size() == pipeline.getReplicationConfig() - .getRequiredNodes(), - "Nodes size=%s, replication factor=%s do not match ", - pipeline.getNodes().size(), pipeline.getReplicationConfig() - .getRequiredNodes()); - if (pipelineMap.putIfAbsent(pipeline.getId(), pipeline) != null) { LOG.warn("Duplicate pipeline ID detected. {}", pipeline.getId()); throw new IOException(String From ce49c048cd23b262c3d68140b855221d2a759a3b Mon Sep 17 00:00:00 2001 From: Uma Maheswara Rao G Date: Fri, 27 May 2022 00:24:22 -0700 Subject: [PATCH 2/4] Added validation check in pipelineFactory#create --- .../hdds/scm/pipeline/PipelineFactory.java | 20 +++++++++++++++++-- .../scm/pipeline/PipelineManagerImpl.java | 12 ----------- .../hdds/scm/pipeline/PipelineStateMap.java | 8 ++++++++ 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineFactory.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineFactory.java index 780a4ee714cb..21a47dee8608 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineFactory.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineFactory.java @@ -85,9 +85,25 @@ public Pipeline create( ReplicationConfig replicationConfig, List excludedNodes, List favoredNodes) throws IOException { - return providers - .get(replicationConfig.getReplicationType()) + Pipeline pipeline = providers.get(replicationConfig.getReplicationType()) .create(replicationConfig, excludedNodes, favoredNodes); + checkPipeline(pipeline); + return pipeline; + } + + private void checkPipeline(Pipeline pipeline) throws IOException { + // In case in case if provided pipeline provider returns null. + if (pipeline == null) { + throw new IOException("Pipeline cannot be null"); + } + // In case if provided pipeline returns less number of nodes than + // required. + if (pipeline.getNodes().size() != pipeline.getReplicationConfig() + .getRequiredNodes()) { + throw new IOException("Nodes size= " + pipeline.getNodes() + .size() + ", replication factor=" + pipeline.getReplicationConfig() + .getRequiredNodes() + " do not match"); + } } public Pipeline create(ReplicationConfig replicationConfig, diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java index 72dd7c272f70..4cb96b1d2fd4 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java @@ -214,18 +214,6 @@ public Pipeline createPipeline(ReplicationConfig replicationConfig, try { Pipeline pipeline = pipelineFactory.create(replicationConfig, excludedNodes, favoredNodes); - // In case in case if provided pipeline provider returns null. - if (pipeline == null) { - throw new IOException("Pipeline cannot be null"); - } - // In case if provided pipeline returns less number of nodes than - // required. - if (pipeline.getNodes().size() != pipeline.getReplicationConfig() - .getRequiredNodes()) { - throw new IOException("Nodes size= " + pipeline.getNodes() - .size() + ", replication factor=" + pipeline.getReplicationConfig() - .getRequiredNodes() + " do not match"); - } stateManager.addPipeline(pipeline.getProtobufMessage( ClientVersion.CURRENT_VERSION)); recordMetricsForPipeline(pipeline); diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java index e61d50296496..5ebc92bb5536 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java @@ -70,6 +70,14 @@ class PipelineStateMap { * @throws IOException if pipeline with provided pipelineID already exists */ void addPipeline(Pipeline pipeline) throws IOException { + Preconditions.checkNotNull(pipeline, "Pipeline cannot be null"); + Preconditions.checkArgument( + pipeline.getNodes().size() == pipeline.getReplicationConfig() + .getRequiredNodes(), + "Nodes size=%s, replication factor=%s do not match ", + pipeline.getNodes().size(), pipeline.getReplicationConfig() + .getRequiredNodes()); + if (pipelineMap.putIfAbsent(pipeline.getId(), pipeline) != null) { LOG.warn("Duplicate pipeline ID detected. {}", pipeline.getId()); throw new IOException(String From ca6cb669486bbac0bf8251cc99c0a2cc05f7e67d Mon Sep 17 00:00:00 2001 From: Uma Maheswara Rao G Date: Fri, 27 May 2022 00:50:19 -0700 Subject: [PATCH 3/4] Corrected a format --- .../org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java index 5ebc92bb5536..6b40f28fc0b1 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java @@ -75,8 +75,8 @@ void addPipeline(Pipeline pipeline) throws IOException { pipeline.getNodes().size() == pipeline.getReplicationConfig() .getRequiredNodes(), "Nodes size=%s, replication factor=%s do not match ", - pipeline.getNodes().size(), pipeline.getReplicationConfig() - .getRequiredNodes()); + pipeline.getNodes().size(), pipeline.getReplicationConfig() + .getRequiredNodes()); if (pipelineMap.putIfAbsent(pipeline.getId(), pipeline) != null) { LOG.warn("Duplicate pipeline ID detected. {}", pipeline.getId()); From 362b1b58dd258ef974b081e59fd3b24e4e97fa91 Mon Sep 17 00:00:00 2001 From: Uma Maheswara Rao G Date: Thu, 2 Jun 2022 07:31:33 -0700 Subject: [PATCH 4/4] Fixed the comments. --- .../algorithms/SCMContainerPlacementRackScatter.java | 10 ++++++++++ .../hadoop/hdds/scm/pipeline/PipelineFactory.java | 10 ++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java index 6c5fea196937..25105497c53e 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java @@ -227,6 +227,16 @@ public List chooseDatanodes( placementStatus.expectedPlacementCount(); throw new SCMException(errorMsg, null); } + if (nodesRequiredToChoose != chosenNodes.size()) { + String reason = "Chosen nodes size: " + chosenNodes + .size() + ", but required nodes to choose: " + nodesRequiredToChoose + + " do not match."; + LOG.warn("Placement policy could not choose the enough nodes." + + " {} Available nodes count: {}, Excluded nodes count: {}", + reason, totalNodesCount, excludedNodesCount); + throw new SCMException(reason, + SCMException.ResultCodes.FAILED_TO_FIND_HEALTHY_NODES); + } return chosenNodes; } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineFactory.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineFactory.java index 21a47dee8608..604c3466c340 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineFactory.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineFactory.java @@ -94,15 +94,17 @@ public Pipeline create( private void checkPipeline(Pipeline pipeline) throws IOException { // In case in case if provided pipeline provider returns null. if (pipeline == null) { - throw new IOException("Pipeline cannot be null"); + throw new SCMException("Pipeline cannot be null", + SCMException.ResultCodes.INTERNAL_ERROR); } // In case if provided pipeline returns less number of nodes than // required. if (pipeline.getNodes().size() != pipeline.getReplicationConfig() .getRequiredNodes()) { - throw new IOException("Nodes size= " + pipeline.getNodes() - .size() + ", replication factor=" + pipeline.getReplicationConfig() - .getRequiredNodes() + " do not match"); + throw new SCMException("Nodes size= " + pipeline.getNodes() + .size() + ", replication factor= " + pipeline.getReplicationConfig() + .getRequiredNodes() + " do not match", + SCMException.ResultCodes.FAILED_TO_FIND_HEALTHY_NODES); } }