You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ozone.apache.org by ck...@apache.org on 2022/10/09 07:43:19 UTC
[ozone] branch master updated: HDDS-7286. Clean up ContainerManager (#3797)
This is an automated email from the ASF dual-hosted git repository.
ckj 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 625e84f95e HDDS-7286. Clean up ContainerManager (#3797)
625e84f95e is described below
commit 625e84f95ef85dccaa9ad087b75e5da85e1f484a
Author: Maxim Myskov <ma...@gmail.com>
AuthorDate: Sun Oct 9 10:43:13 2022 +0300
HDDS-7286. Clean up ContainerManager (#3797)
---
.../hdds/scm/container/ContainerManager.java | 14 ++---------
.../hdds/scm/container/ContainerManagerImpl.java | 27 +++-------------------
.../scm/container/TestContainerManagerImpl.java | 12 +++++-----
.../TestContainerStateManagerIntegration.java | 2 +-
.../hdds/scm/storage/TestContainerCommandsEC.java | 7 +++++-
.../hadoop/ozone/recon/TestReconAsPassiveScm.java | 4 ++--
.../apache/hadoop/ozone/recon/TestReconTasks.java | 4 ++--
.../ozone/recon/scm/ReconContainerManager.java | 27 +++++++---------------
8 files changed, 30 insertions(+), 67 deletions(-)
diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManager.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManager.java
index 247f1c3b85..fb171677f0 100644
--- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManager.java
+++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManager.java
@@ -32,14 +32,10 @@ import org.apache.hadoop.hdds.utils.db.Table;
import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
/**
- * TODO: Add extensive javadoc.
- *
- * ContainerManager class contains the mapping from a name to a pipeline
- * mapping. This is used by SCM when allocating new locations and when
- * looking up a key.
+ * ContainerManager is responsible for keeping track of all Containers and
+ * managing all containers operations like creating, deleting etc.
*/
public interface ContainerManager extends Closeable {
- // TODO: Rename this to ContainerManager
/**
* Reinitialize the containerManager with the updated container store.
@@ -191,12 +187,6 @@ public interface ContainerManager extends Closeable {
void deleteContainer(ContainerID containerID)
throws IOException, TimeoutException;
- /**
- * Returns the list of containersIDs.
- * @return list of containerIDs
- */
- Set<ContainerID> getContainerIDs();
-
/**
* Returns containerStateManger.
* @return containerStateManger
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 6a76f02c4c..9314d07de3 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
@@ -58,34 +58,20 @@ import org.slf4j.LoggerFactory;
import static org.apache.hadoop.hdds.scm.ha.SequenceIdGenerator.CONTAINER_ID;
/**
- * TODO: Add javadoc.
+ * {@link ContainerManager} implementation in SCM server.
*/
public class ContainerManagerImpl implements ContainerManager {
- /*
- * TODO: Introduce container level locks.
- */
-
- /**
- *
- */
private static final Logger LOG = LoggerFactory.getLogger(
ContainerManagerImpl.class);
/**
- *
+ * Limit the number of on-going ratis operations.
*/
- // Limit the number of on-going ratis operations.
private final Lock lock;
- /**
- *
- */
private final PipelineManager pipelineManager;
- /**
- *
- */
private final ContainerStateManager containerStateManager;
private final SCMHAManager haManager;
@@ -162,9 +148,6 @@ public class ContainerManagerImpl implements ContainerManager {
public List<ContainerInfo> getContainers(final ContainerID startID,
final int count) {
scmContainerManagerMetrics.incNumListContainersOps();
- // TODO: Remove the null check, startID should not be null. Fix the unit
- // test before removing the check.
- final long start = startID == null ? 0 : startID.getId();
final List<ContainerID> containersIds =
new ArrayList<>(containerStateManager.getContainerIDs());
Collections.sort(containersIds);
@@ -172,7 +155,7 @@ public class ContainerManagerImpl implements ContainerManager {
lock.lock();
try {
containers = containersIds.stream()
- .filter(id -> id.getId() >= start).limit(count)
+ .filter(id -> id.compareTo(startID) >= 0).limit(count)
.map(containerStateManager::getContainer)
.collect(Collectors.toList());
} finally {
@@ -478,8 +461,4 @@ public class ContainerManagerImpl implements ContainerManager {
public SCMHAManager getSCMHAManager() {
return haManager;
}
-
- public Set<ContainerID> getContainerIDs() {
- return containerStateManager.getContainerIDs();
- }
}
diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerManagerImpl.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerManagerImpl.java
index 7c0a59e070..ffe725ea2b 100644
--- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerManagerImpl.java
+++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerManagerImpl.java
@@ -101,7 +101,7 @@ public class TestContainerManagerImpl {
}
@Test
- public void testAllocateContainer() throws Exception {
+ void testAllocateContainer() throws Exception {
Assertions.assertTrue(
containerManager.getContainers().isEmpty());
final ContainerInfo container = containerManager.allocateContainer(
@@ -113,7 +113,7 @@ public class TestContainerManagerImpl {
}
@Test
- public void testUpdateContainerState() throws Exception {
+ void testUpdateContainerState() throws Exception {
final ContainerInfo container = containerManager.allocateContainer(
RatisReplicationConfig.getInstance(
ReplicationFactor.THREE), "admin");
@@ -135,7 +135,7 @@ public class TestContainerManagerImpl {
}
@Test
- public void testGetContainers() throws Exception {
+ void testGetContainers() throws Exception {
Assertions.assertTrue(
containerManager.getContainers().isEmpty());
@@ -181,7 +181,7 @@ public class TestContainerManagerImpl {
}
@Test
- public void testAllocateContainersWithECReplicationConfig() throws Exception {
+ void testAllocateContainersWithECReplicationConfig() throws Exception {
final ContainerInfo admin = containerManager
.allocateContainer(new ECReplicationConfig(3, 2), "admin");
Assertions.assertEquals(1, containerManager.getContainers().size());
@@ -190,7 +190,7 @@ public class TestContainerManagerImpl {
}
@Test
- public void testUpdateContainerReplicaInvokesPendingOp()
+ void testUpdateContainerReplicaInvokesPendingOp()
throws IOException, TimeoutException {
final ContainerInfo container = containerManager.allocateContainer(
RatisReplicationConfig.getInstance(
@@ -211,7 +211,7 @@ public class TestContainerManagerImpl {
}
@Test
- public void testRemoveContainerReplicaInvokesPendingOp()
+ void testRemoveContainerReplicaInvokesPendingOp()
throws IOException, TimeoutException {
final ContainerInfo container = containerManager.allocateContainer(
RatisReplicationConfig.getInstance(
diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerStateManagerIntegration.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerStateManagerIntegration.java
index 81bb05496f..d7f1156690 100644
--- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerStateManagerIntegration.java
+++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerStateManagerIntegration.java
@@ -166,7 +166,7 @@ public class TestContainerStateManagerIntegration {
cluster.restartStorageContainerManager(false);
List<ContainerInfo> result = cluster.getStorageContainerManager()
- .getContainerManager().getContainers(null, 100);
+ .getContainerManager().getContainers();
long matchCount = result.stream()
.filter(info ->
diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/storage/TestContainerCommandsEC.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/storage/TestContainerCommandsEC.java
index 61efbeadb1..8713dc2a17 100644
--- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/storage/TestContainerCommandsEC.java
+++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/storage/TestContainerCommandsEC.java
@@ -658,8 +658,13 @@ public class TestContainerCommandsEC {
out.write(values[i]);
}
}
+// List<ContainerID> containerIDs =
+// new ArrayList<>(scm.getContainerManager().getContainerIDs());
List<ContainerID> containerIDs =
- new ArrayList<>(scm.getContainerManager().getContainerIDs());
+ scm.getContainerManager().getContainers()
+ .stream()
+ .map(ContainerInfo::containerID)
+ .collect(Collectors.toList());
Assertions.assertEquals(1, containerIDs.size());
containerID = containerIDs.get(0).getId();
List<Pipeline> pipelines = scm.getPipelineManager().getPipelines(repConfig);
diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconAsPassiveScm.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconAsPassiveScm.java
index 1dd80f3881..c02f4cc58a 100644
--- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconAsPassiveScm.java
+++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconAsPassiveScm.java
@@ -138,8 +138,8 @@ public class TestReconAsPassiveScm {
runTestOzoneContainerViaDataNode(containerID, client);
// Verify Recon picked up the new container that was created.
- assertEquals(scmContainerManager.getContainerIDs(),
- reconContainerManager.getContainerIDs());
+ assertEquals(scmContainerManager.getContainers(),
+ reconContainerManager.getContainers());
GenericTestUtils.LogCapturer logCapturer =
GenericTestUtils.LogCapturer.captureLogs(ReconNodeManager.LOG);
diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconTasks.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconTasks.java
index cd9675e70a..abeb1f189c 100644
--- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconTasks.java
+++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconTasks.java
@@ -115,8 +115,8 @@ public class TestReconTasks {
runTestOzoneContainerViaDataNode(containerID, client);
// Make sure Recon got the container report with new container.
- Assert.assertEquals(scmContainerManager.getContainerIDs(),
- reconContainerManager.getContainerIDs());
+ Assert.assertEquals(scmContainerManager.getContainers(),
+ reconContainerManager.getContainers());
// Bring down the Datanode that had the container replica.
cluster.shutdownHddsDatanode(pipeline.getFirstNode());
diff --git a/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerManager.java b/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerManager.java
index cccd48124d..2fdcb91a44 100644
--- a/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerManager.java
+++ b/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerManager.java
@@ -50,6 +50,7 @@ import org.apache.hadoop.hdds.scm.pipeline.PipelineID;
import org.apache.hadoop.hdds.scm.pipeline.PipelineManager;
import org.apache.hadoop.hdds.utils.db.DBStore;
import org.apache.hadoop.hdds.utils.db.Table;
+import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
import org.apache.hadoop.ozone.recon.persistence.ContainerHistory;
import org.apache.hadoop.ozone.recon.persistence.ContainerHealthSchemaManager;
import org.apache.hadoop.ozone.recon.spi.ReconContainerMetadataManager;
@@ -65,8 +66,8 @@ public class ReconContainerManager extends ContainerManagerImpl {
private static final Logger LOG =
LoggerFactory.getLogger(ReconContainerManager.class);
- private StorageContainerServiceProvider scmClient;
- private PipelineManager pipelineManager;
+ private final StorageContainerServiceProvider scmClient;
+ private final PipelineManager pipelineManager;
private final ContainerHealthSchemaManager containerHealthSchemaManager;
private final ReconContainerMetadataManager cdbServiceProvider;
private final Table<UUID, DatanodeDetails> nodeDB;
@@ -75,16 +76,6 @@ public class ReconContainerManager extends ContainerManagerImpl {
// Pipeline -> # of open containers
private final Map<PipelineID, Integer> pipelineToOpenContainer;
- /**
- * Constructs a mapping class that creates mapping between container names
- * and pipelines.
- * <p>
- * passed to LevelDB and this memory is allocated in Native code space.
- * CacheSize is specified
- * in MB.
- *
- * @throws IOException on Failure.
- */
@SuppressWarnings("parameternumber")
public ReconContainerManager(
Configuration conf,
@@ -104,7 +95,6 @@ public class ReconContainerManager extends ContainerManagerImpl {
this.pipelineManager = pipelineManager;
this.containerHealthSchemaManager = containerHealthSchemaManager;
this.cdbServiceProvider = reconContainerMetadataManager;
- // batchHandler = scmDBStore
this.nodeDB = ReconSCMDBDefinition.NODES.getTable(store);
this.replicaHistoryMap = new ConcurrentHashMap<>();
this.pipelineToOpenContainer = new ConcurrentHashMap<>();
@@ -120,7 +110,7 @@ public class ReconContainerManager extends ContainerManagerImpl {
public void checkAndAddNewContainer(ContainerID containerID,
ContainerReplicaProto.State replicaState,
DatanodeDetails datanodeDetails)
- throws Exception {
+ throws IOException, TimeoutException, InvalidStateTransitionException {
if (!containerExist(containerID)) {
LOG.info("New container {} got from {}.", containerID,
datanodeDetails.getHostName());
@@ -198,12 +188,12 @@ public class ReconContainerManager extends ContainerManagerImpl {
*
* @param containerID containerID to check
* @param state state to be compared
- * @throws IOException
*/
private void checkContainerStateAndUpdate(ContainerID containerID,
ContainerReplicaProto.State state)
- throws Exception {
+ throws IOException, InvalidStateTransitionException,
+ TimeoutException {
ContainerInfo containerInfo = getContainer(containerID);
if (containerInfo.getState().equals(HddsProtos.LifeCycleState.OPEN)
&& !state.equals(ContainerReplicaProto.State.OPEN)
@@ -252,9 +242,8 @@ public class ReconContainerManager extends ContainerManagerImpl {
} else {
// Get open container for a pipeline that Recon does not know
// about yet. Cannot update internal state until pipeline is synced.
- LOG.warn(String.format(
- "Pipeline %s not found. Cannot add container %s",
- pipelineID, containerInfo.containerID()));
+ LOG.warn("Pipeline {} not found. Cannot add container {}",
+ pipelineID, containerInfo.containerID());
}
} else {
getContainerStateManager().addContainer(containerInfo.getProtobuf());
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@ozone.apache.org
For additional commands, e-mail: commits-help@ozone.apache.org