This is an automated email from the ASF dual-hosted git repository.
adoroszlai pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ozone.git
The following commit(s) were added to refs/heads/master by this push:
new 5d5eb45d14 Revert "HDDS-9151. Close EC Pipeline when container
transitions to closing (#5191)" (#5270)
5d5eb45d14 is described below
commit 5d5eb45d14952056faaf2150db273a2f152ff867
Author: Stephen O'Donnell <[email protected]>
AuthorDate: Tue Sep 12 15:24:08 2023 +0100
Revert "HDDS-9151. Close EC Pipeline when container transitions to closing
(#5191)" (#5270)
This reverts commit 77dc4dbf00b609610b55b3df28839d77804e3956.
---
.../hdds/scm/container/ContainerManagerImpl.java | 51 ++++++++----------
.../hdds/scm/pipeline/PipelineManagerImpl.java | 23 +-------
.../hdds/scm/pipeline/TestPipelineManagerImpl.java | 61 ++--------------------
3 files changed, 27 insertions(+), 108 deletions(-)
diff --git
a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java
index b0e8b986fc..34604edb3b 100644
---
a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java
+++
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java
@@ -172,27 +172,24 @@ public class ContainerManagerImpl implements
ContainerManager {
public ContainerInfo allocateContainer(
final ReplicationConfig replicationConfig, final String owner)
throws IOException {
+ // Acquire pipeline manager lock, to avoid any updates to pipeline
+ // while allocate container happens. This is to avoid scenario like
+ // mentioned in HDDS-5655.
+ pipelineManager.acquireReadLock();
+ lock.lock();
List<Pipeline> pipelines;
Pipeline pipeline;
ContainerInfo containerInfo = null;
- lock.lock();
try {
- // Acquire pipeline manager lock, to avoid any updates to pipeline
- // while allocate container happens. This is to avoid scenario like
- // mentioned in HDDS-5655.
- pipelineManager.acquireReadLock();
- try {
- pipelines = pipelineManager
- .getPipelines(replicationConfig, Pipeline.PipelineState.OPEN);
- if (!pipelines.isEmpty()) {
- pipeline = pipelines.get(random.nextInt(pipelines.size()));
- containerInfo = createContainer(pipeline, owner);
- }
- } finally {
- pipelineManager.releaseReadLock();
+ pipelines = pipelineManager
+ .getPipelines(replicationConfig, Pipeline.PipelineState.OPEN);
+ if (!pipelines.isEmpty()) {
+ pipeline = pipelines.get(random.nextInt(pipelines.size()));
+ containerInfo = createContainer(pipeline, owner);
}
} finally {
lock.unlock();
+ pipelineManager.releaseReadLock();
}
if (pipelines.isEmpty()) {
@@ -205,26 +202,22 @@ public class ContainerManagerImpl implements
ContainerManager {
" matching pipeline for replicationConfig: " + replicationConfig
+ ", State:PipelineState.OPEN", e);
}
-
+ pipelineManager.acquireReadLock();
lock.lock();
try {
- pipelineManager.acquireReadLock();
- try {
- pipelines = pipelineManager
- .getPipelines(replicationConfig, Pipeline.PipelineState.OPEN);
- if (!pipelines.isEmpty()) {
- pipeline = pipelines.get(random.nextInt(pipelines.size()));
- containerInfo = createContainer(pipeline, owner);
- } else {
- throw new IOException("Could not allocate container. Cannot get " +
- " any matching pipeline for replicationConfig: " +
- replicationConfig + ", State:PipelineState.OPEN");
- }
- } finally {
- pipelineManager.releaseReadLock();
+ pipelines = pipelineManager
+ .getPipelines(replicationConfig, Pipeline.PipelineState.OPEN);
+ if (!pipelines.isEmpty()) {
+ pipeline = pipelines.get(random.nextInt(pipelines.size()));
+ containerInfo = createContainer(pipeline, owner);
+ } else {
+ throw new IOException("Could not allocate container. Cannot get any"
+
+ " matching pipeline for replicationConfig: " + replicationConfig
+ + ", State:PipelineState.OPEN");
}
} finally {
lock.unlock();
+ pipelineManager.releaseReadLock();
}
}
return containerInfo;
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 470491379e..d5cb5504eb 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
@@ -400,27 +400,8 @@ public class PipelineManagerImpl implements
PipelineManager {
@Override
public void removeContainerFromPipeline(
PipelineID pipelineID, ContainerID containerID) throws IOException {
- acquireWriteLock();
- try {
- Pipeline pipeline = stateManager.getPipeline(pipelineID);
- stateManager.removeContainerFromPipeline(pipelineID, containerID);
- if (pipeline.getReplicationConfig().getReplicationType()
- == ReplicationType.EC) {
- // For EC, a pipeline is used for only a single container. When that
- // container is closed or removed from the pipeline, the pipeline
should
- // also be closed. For EC, if a pipeline had a container and then the
- // container is removed via this method, the pipeline is no longer
- // useful - nothing will allocate a new container on it. Therefore, we
- // close the pipeline here to free up the pipeline slot for a new
- // pipeline to get created
- closePipeline(pipeline, true);
- }
- } catch (PipelineNotFoundException e) {
- LOG.warn("Pipeline {} not found when removing container {}",
- pipelineID, containerID, e);
- } finally {
- releaseWriteLock();
- }
+ // should not lock here, since no ratis operation happens.
+ stateManager.removeContainerFromPipeline(pipelineID, containerID);
}
@Override
diff --git
a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelineManagerImpl.java
b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelineManagerImpl.java
index ea6d13b183..33243b650e 100644
---
a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelineManagerImpl.java
+++
b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelineManagerImpl.java
@@ -87,7 +87,6 @@ import static
org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_DATANODE_PIPELINE_L
import static
org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_DATANODE_PIPELINE_LIMIT_DEFAULT;
import static
org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_PIPELINE_ALLOCATED_TIMEOUT;
import static
org.apache.hadoop.hdds.scm.pipeline.Pipeline.PipelineState.ALLOCATED;
-import static
org.apache.hadoop.hdds.scm.pipeline.Pipeline.PipelineState.CLOSED;
import static org.apache.hadoop.hdds.scm.pipeline.Pipeline.PipelineState.OPEN;
import static org.apache.hadoop.test.MetricsAsserts.getLongCounter;
import static org.apache.hadoop.test.MetricsAsserts.getMetrics;
@@ -353,60 +352,6 @@ public class TestPipelineManagerImpl {
}
}
- @Test
- public void testRemoveContainerFromPipeline() throws Exception {
- try (PipelineManagerImpl pipelineManager = createPipelineManager(true)) {
- Pipeline ratisPipeline = pipelineManager.createPipeline(
- RatisReplicationConfig.getInstance(ReplicationFactor.THREE));
- pipelineManager.openPipeline(ratisPipeline.getId());
- ECReplicationConfig ecRepConfig = new ECReplicationConfig(3, 2);
- Pipeline ecPipeline = pipelineManager.createPipeline(ecRepConfig);
- pipelineManager.openPipeline(ecPipeline.getId());
-
- ContainerInfo ratisContainerInfo = HddsTestUtils.
- getContainer(HddsProtos.LifeCycleState.OPEN, ratisPipeline.getId());
- ContainerInfo ecContainerInfo = HddsTestUtils.getECContainer(
- HddsProtos.LifeCycleState.OPEN, ecPipeline.getId(), ecRepConfig);
-
- pipelineManager.addContainerToPipeline(ratisPipeline.getId(),
- ratisContainerInfo.containerID());
- pipelineManager.addContainerToPipeline(ecPipeline.getId(),
- ecContainerInfo.containerID());
-
- List<Pipeline> ratisPipelines = pipelineManager.getPipelines(
- RatisReplicationConfig.getInstance(ReplicationFactor.THREE),
- Pipeline.PipelineState.OPEN);
-
- List<Pipeline> ecPipelines = pipelineManager.getPipelines(
- ecRepConfig, Pipeline.PipelineState.OPEN);
-
- // Background pipeline creator could create additional Ratis pipelines,
- // so an equals check may not work. EC pipelines are created on demand,
- // so an equals check is fine.
- Assertions.assertTrue(ratisPipelines.contains(ratisPipeline));
- Assertions.assertEquals(1, ecPipelines.size());
-
- // Ensure both pipelines have a single container
- Assertions.assertEquals(1, pipelineManager
- .getContainersInPipeline(ratisPipeline.getId()).size());
- Assertions.assertEquals(1, pipelineManager
- .getContainersInPipeline(ecPipeline.getId()).size());
-
- // Remove the container from the Ratis pipeline - this should leave the
- // pipeline in the OPEN state
- pipelineManager.removeContainerFromPipeline(ratisPipeline.getId(),
- ratisContainerInfo.containerID());
- Assertions.assertEquals(OPEN, pipelineManager
- .getPipeline(ratisPipeline.getId()).getPipelineState());
-
- // Removing from the EC pipeline should trigger the pipeline to close.
- pipelineManager.removeContainerFromPipeline(ecPipeline.getId(),
- ecContainerInfo.containerID());
- Assertions.assertEquals(CLOSED, pipelineManager
- .getPipeline(ecPipeline.getId()).getPipelineState());
- }
- }
-
@Test
public void testClosePipelineShouldFailOnFollower() throws Exception {
try (PipelineManagerImpl pipelineManager = createPipelineManager(true)) {
@@ -594,7 +539,7 @@ public class TestPipelineManagerImpl {
Assertions.assertTrue(pipelineManager
.getPipelines(RatisReplicationConfig
.getInstance(ReplicationFactor.THREE),
- CLOSED).contains(closedPipeline));
+ Pipeline.PipelineState.CLOSED).contains(closedPipeline));
// Set the clock to "now". All pipelines were created before this.
testClock.set(Instant.now());
@@ -612,7 +557,7 @@ public class TestPipelineManagerImpl {
Assertions.assertFalse(pipelineManager
.getPipelines(RatisReplicationConfig
.getInstance(ReplicationFactor.THREE),
- CLOSED).contains(closedPipeline));
+ Pipeline.PipelineState.CLOSED).contains(closedPipeline));
testClock.fastForward((60000));
@@ -647,7 +592,7 @@ public class TestPipelineManagerImpl {
pipelineManager.scrubPipelines();
pipeline = pipelineManager.getPipeline(pipeline.getId());
- Assertions.assertEquals(CLOSED,
+ Assertions.assertEquals(Pipeline.PipelineState.CLOSED,
pipeline.getPipelineState());
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
