From d9193ae09d300593dfb91cc46ea1143c7bcaef36 Mon Sep 17 00:00:00 2001 From: Bharat Viswanadham Date: Thu, 21 Feb 2019 11:43:19 -0800 Subject: [PATCH 1/2] HDDS-1148.After allocating container, we are not adding to container DB. --- .../scm/container/ContainerStateManager.java | 84 ++---------- .../scm/container/SCMContainerManager.java | 125 +++++++++++++++--- 2 files changed, 119 insertions(+), 90 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java index c182d0acae5e2..423ac782f216d 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java @@ -121,7 +121,6 @@ public class ContainerStateManager { private final ConcurrentHashMap lastUsedMap; private final ContainerStateMap containers; private final AtomicLong containerCount; - private final int numContainerPerOwnerInPipeline; /** * Constructs a Container State Manager that tracks all containers owned by @@ -152,9 +151,6 @@ public ContainerStateManager(final Configuration configuration) { this.lastUsedMap = new ConcurrentHashMap<>(); this.containerCount = new AtomicLong(0); this.containers = new ContainerStateMap(); - this.numContainerPerOwnerInPipeline = configuration - .getInt(ScmConfigKeys.OZONE_SCM_PIPELINE_OWNER_CONTAINER_COUNT, - ScmConfigKeys.OZONE_SCM_PIPELINE_OWNER_CONTAINER_COUNT_DEFAULT); } /* @@ -362,55 +358,6 @@ void updateDeleteTransactionId( }); } - /** - * Return a container matching the attributes specified. - * - * @param size - Space needed in the Container. - * @param owner - Owner of the container - A specific nameservice. - * @param pipelineManager - Pipeline Manager - * @param pipeline - Pipeline from which container needs to be matched - * @return ContainerInfo, null if there is no match found. - */ - ContainerInfo getMatchingContainer(final long size, String owner, - PipelineManager pipelineManager, Pipeline pipeline) throws IOException { - - NavigableSet containerIDs = - pipelineManager.getContainersInPipeline(pipeline.getId()); - if (containerIDs == null) { - LOG.error("Container list is null for pipeline=", pipeline.getId()); - return null; - } - - getContainers(containerIDs, owner); - if (containerIDs.size() < numContainerPerOwnerInPipeline) { - synchronized (pipeline) { - // TODO: #CLUTIL Maybe we can add selection logic inside synchronized - // as well - containerIDs = getContainers( - pipelineManager.getContainersInPipeline(pipeline.getId()), owner); - if (containerIDs.size() < numContainerPerOwnerInPipeline) { - ContainerInfo containerInfo = - allocateContainer(pipelineManager, owner, pipeline); - lastUsedMap.put(new ContainerState(owner, pipeline.getId()), - containerInfo.containerID()); - return containerInfo; - } - } - } - - ContainerInfo containerInfo = - getMatchingContainer(size, owner, pipeline.getId(), containerIDs); - if (containerInfo == null) { - synchronized (pipeline) { - containerInfo = - allocateContainer(pipelineManager, owner, pipeline); - lastUsedMap.put(new ContainerState(owner, pipeline.getId()), - containerInfo.containerID()); - } - } - // TODO: #CLUTIL cleanup entries in lastUsedMap - return containerInfo; - } /** * Return a container matching the attributes specified. @@ -469,9 +416,6 @@ private ContainerInfo findContainerWithSpace(final long size, final ContainerInfo containerInfo = containers.getContainerInfo(id); if (containerInfo.getUsedBytes() + size <= this.containerSize) { containerInfo.updateLastUsedTime(); - - final ContainerState key = new ContainerState(owner, pipelineID); - lastUsedMap.put(key, containerInfo.containerID()); return containerInfo; } } @@ -523,21 +467,7 @@ ContainerInfo getContainer(final ContainerID containerID) return containers.getContainerInfo(containerID); } - private NavigableSet getContainers( - NavigableSet containerIDs, String owner) { - for (ContainerID cid : containerIDs) { - try { - if (!getContainer(cid).getOwner().equals(owner)) { - containerIDs.remove(cid); - } - } catch (ContainerNotFoundException e) { - LOG.error("Could not find container info for container id={} {}", cid, - e); - containerIDs.remove(cid); - } - } - return containerIDs; - } + void close() throws IOException { } @@ -583,4 +513,16 @@ void removeContainer(final ContainerID containerID) containers.removeContainer(containerID); } + /** + * Update the lastUsedmap to update with ContainerState and containerID. + * @param pipelineID + * @param containerID + * @param owner + */ + public synchronized void updateLastUsedMap(PipelineID pipelineID, + ContainerID containerID, String owner) { + lastUsedMap.put(new ContainerState(owner, pipelineID), + containerID); + } + } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/SCMContainerManager.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/SCMContainerManager.java index 93abdcc828da2..9c23bb054d74b 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/SCMContainerManager.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/SCMContainerManager.java @@ -22,6 +22,7 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hdds.protocol.proto.HddsProtos.ContainerInfoProto; import org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState; +import org.apache.hadoop.hdds.scm.ScmConfigKeys; import org.apache.hadoop.hdds.scm.exceptions.SCMException; import org.apache.hadoop.hdds.scm.node.NodeManager; import org.apache.hadoop.hdds.scm.pipeline.Pipeline; @@ -44,6 +45,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.NavigableSet; import java.util.Objects; import java.util.Set; import java.util.concurrent.locks.Lock; @@ -67,6 +69,7 @@ public class SCMContainerManager implements ContainerManager { private final MetadataStore containerStore; private final PipelineManager pipelineManager; private final ContainerStateManager containerStateManager; + private final int numContainerPerOwnerInPipeline; /** * Constructs a mapping class that creates mapping between container names @@ -100,6 +103,9 @@ public SCMContainerManager(final Configuration conf, this.lock = new ReentrantLock(); this.pipelineManager = pipelineManager; this.containerStateManager = new ContainerStateManager(conf); + this.numContainerPerOwnerInPipeline = conf + .getInt(ScmConfigKeys.OZONE_SCM_PIPELINE_OWNER_CONTAINER_COUNT, + ScmConfigKeys.OZONE_SCM_PIPELINE_OWNER_CONTAINER_COUNT_DEFAULT); loadExistingContainers(); } @@ -201,6 +207,7 @@ public List listContainer(ContainerID startContainerID, } } + /** * Allocates a new container. * @@ -215,23 +222,11 @@ public ContainerInfo allocateContainer(final ReplicationType type, throws IOException { lock.lock(); try { - final ContainerInfo containerInfo; containerInfo = containerStateManager + final ContainerInfo containerInfo; + containerInfo = containerStateManager .allocateContainer(pipelineManager, type, replicationFactor, owner); - try { - final byte[] containerIDBytes = Longs.toByteArray( - containerInfo.getContainerID()); - containerStore.put(containerIDBytes, - containerInfo.getProtobuf().toByteArray()); - } catch (IOException ex) { - // If adding to containerStore fails, we should remove the container - // from in-memory map. - try { - containerStateManager.removeContainer(containerInfo.containerID()); - } catch (ContainerNotFoundException cnfe) { - // No need to worry much, everything is going as planned. - } - throw ex; - } + // Add container to DB. + addContainerToDB(containerInfo); return containerInfo; } finally { lock.unlock(); @@ -360,9 +355,52 @@ public ContainerInfo getMatchingContainer(final long sizeRequired, String owner, Pipeline pipeline) { try { //TODO: #CLUTIL See if lock is required here - return containerStateManager - .getMatchingContainer(sizeRequired, owner, pipelineManager, - pipeline); + NavigableSet containerIDs = + pipelineManager.getContainersInPipeline(pipeline.getId()); + if (containerIDs == null) { + LOG.error("Container list is null for pipeline=", pipeline.getId()); + return null; + } + + containerIDs = getContainersForOwner(containerIDs, owner); + if (containerIDs.size() < numContainerPerOwnerInPipeline) { + synchronized (pipeline) { + // TODO: #CLUTIL Maybe we can add selection logic inside synchronized + // as well + containerIDs = getContainersForOwner( + pipelineManager.getContainersInPipeline(pipeline.getId()), owner); + if (containerIDs.size() < numContainerPerOwnerInPipeline) { + ContainerInfo containerInfo = + containerStateManager.allocateContainer(pipelineManager, owner, + pipeline); + // Add to DB + addContainerToDB(containerInfo); + containerStateManager.updateLastUsedMap(pipeline.getId(), + containerInfo.containerID(), owner); + return containerInfo; + } + } + } + + ContainerInfo containerInfo = + containerStateManager.getMatchingContainer(sizeRequired, owner, + pipeline.getId(), containerIDs); + if (containerInfo == null) { + synchronized (pipeline) { + containerInfo = + containerStateManager.allocateContainer(pipelineManager, owner, + pipeline); + // Add to DB + addContainerToDB(containerInfo); + containerStateManager.updateLastUsedMap(pipeline.getId(), + containerInfo.containerID(), owner); + } + } else { + containerStateManager.updateLastUsedMap(pipeline.getId(), + containerInfo.containerID(), owner); + } + // TODO: #CLUTIL cleanup entries in lastUsedMap + return containerInfo; } catch (Exception e) { LOG.warn("Container allocation failed for pipeline={} requiredSize={} {}", pipeline, sizeRequired, e); @@ -370,6 +408,55 @@ public ContainerInfo getMatchingContainer(final long sizeRequired, } } + /** + * Add newly allocated container to container DB. + * @param containerInfo + * @throws IOException + */ + private void addContainerToDB(ContainerInfo containerInfo) + throws IOException { + try { + final byte[] containerIDBytes = Longs.toByteArray( + containerInfo.getContainerID()); + containerStore.put(containerIDBytes, + containerInfo.getProtobuf().toByteArray()); + } catch (IOException ex) { + // If adding to containerStore fails, we should remove the container + // from in-memory map. + try { + containerStateManager.removeContainer(containerInfo.containerID()); + } catch (ContainerNotFoundException cnfe) { + // This should not happen, as we are removing after adding in to + // container state cmap. + } + throw ex; + } + } + + /** + * Returns the container ID's matching with specified owner. + * @param containerIDs + * @param owner + * @return NavigableSet + */ + private NavigableSet getContainersForOwner( + NavigableSet containerIDs, String owner) { + for (ContainerID cid : containerIDs) { + try { + if (!getContainer(cid).getOwner().equals(owner)) { + containerIDs.remove(cid); + } + } catch (ContainerNotFoundException e) { + LOG.error("Could not find container info for container id={} {}", cid, + e); + containerIDs.remove(cid); + } + } + return containerIDs; + } + + + /** * Returns the latest list of DataNodes where replica for given containerId * exist. Throws an SCMException if no entry is found for given containerId. From 44f54be2ea949a4cc2354303bb18e58c1301ab6e Mon Sep 17 00:00:00 2001 From: Bharat Viswanadham Date: Thu, 21 Feb 2019 13:52:26 -0800 Subject: [PATCH 2/2] Addressed review comments --- .../hdds/scm/container/SCMContainerManager.java | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/SCMContainerManager.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/SCMContainerManager.java index 9c23bb054d74b..96c54f31aa9f9 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/SCMContainerManager.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/SCMContainerManager.java @@ -222,9 +222,9 @@ public ContainerInfo allocateContainer(final ReplicationType type, throws IOException { lock.lock(); try { - final ContainerInfo containerInfo; - containerInfo = containerStateManager - .allocateContainer(pipelineManager, type, replicationFactor, owner); + final ContainerInfo containerInfo = + containerStateManager.allocateContainer(pipelineManager, type, + replicationFactor, owner); // Add container to DB. addContainerToDB(containerInfo); return containerInfo; @@ -357,10 +357,6 @@ public ContainerInfo getMatchingContainer(final long sizeRequired, //TODO: #CLUTIL See if lock is required here NavigableSet containerIDs = pipelineManager.getContainersInPipeline(pipeline.getId()); - if (containerIDs == null) { - LOG.error("Container list is null for pipeline=", pipeline.getId()); - return null; - } containerIDs = getContainersForOwner(containerIDs, owner); if (containerIDs.size() < numContainerPerOwnerInPipeline) { @@ -392,13 +388,10 @@ public ContainerInfo getMatchingContainer(final long sizeRequired, pipeline); // Add to DB addContainerToDB(containerInfo); - containerStateManager.updateLastUsedMap(pipeline.getId(), - containerInfo.containerID(), owner); } - } else { - containerStateManager.updateLastUsedMap(pipeline.getId(), - containerInfo.containerID(), owner); } + containerStateManager.updateLastUsedMap(pipeline.getId(), + containerInfo.containerID(), owner); // TODO: #CLUTIL cleanup entries in lastUsedMap return containerInfo; } catch (Exception e) {