siddhantsangwan commented on code in PR #8663:
URL: https://github.com/apache/ozone/pull/8663#discussion_r2158318181
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java:
##########
@@ -346,14 +352,24 @@ public ContainerInfo getMatchingContainer(final long
size, final String owner,
synchronized (pipeline.getId()) {
containerIDs = getContainersForOwner(pipeline, owner);
if (containerIDs.size() < getOpenContainerCountPerPipeline(pipeline)) {
- allocateContainer(pipeline, owner);
- containerIDs = getContainersForOwner(pipeline, owner);
+ if (pipelineManager.hasEnoughSpace(pipeline, containerSize)) {
Review Comment:
Can fix them by doing something like this, what do you think?
```
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 65b06bfd42..2e30b72888 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
@@ -243,6 +243,10 @@ private ContainerInfo createContainer(Pipeline
pipeline, String owner)
private ContainerInfo allocateContainer(final Pipeline pipeline,
final String owner)
throws IOException {
+ if (!pipelineManager.hasEnoughSpace(pipeline, containerSize)) {
+ return null;
+ }
+
final long uniqueId = sequenceIdGen.getNextId(CONTAINER_ID);
Preconditions.checkState(uniqueId > 0,
"Cannot allocate container, negative container id" +
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 baa09cd854..df74709f93 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
@@ -84,7 +84,7 @@ public class TestContainerManagerImpl {
private SequenceIdGenerator sequenceIdGen;
private NodeManager nodeManager;
private ContainerReplicaPendingOps pendingOpsMock;
- private PipelineManager pipelineManager;
+ private MockPipelineManager pipelineManager;
@BeforeAll
static void init() {
@@ -142,6 +142,7 @@ public void
testGetMatchingContainerReturnsNullWhenNotEnoughSpaceInDatanodes() t
Pipeline pipeline = pipelineManager.getPipelines().iterator().next();
// MockPipelineManager#hasEnoughSpace always returns false
// the pipeline has no existing containers, so a new container gets
allocated in getMatchingContainer
+ pipelineManager.setHasEnoughSpaceBehavior(false);
ContainerInfo container = containerManager
.getMatchingContainer(sizeRequired, "test", pipeline,
Collections.emptySet());
assertNull(container);
@@ -159,9 +160,9 @@ public void
testGetMatchingContainerReturnsContainerWhenEnoughSpaceInDatanodes()
long sizeRequired = 256 * 1024 * 1024; // 256 MB
// create a spy to mock hasEnoughSpace to always return true
- PipelineManager spyPipelineManager = spy(pipelineManager);
- doReturn(true).when(spyPipelineManager)
- .hasEnoughSpace(any(Pipeline.class), anyLong());
+ PipelineManager spyPipelineManager = pipelineManager;
+// doReturn(true).when(spyPipelineManager)
+// .hasEnoughSpace(any(Pipeline.class), anyLong());
// create a new ContainerManager using the spy
File tempDir = new File(testDir, "tempDir");
diff --git
a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/MockPipelineManager.java
b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/MockPipelineManager.java
index 78f1865d2b..3274de3d8a 100644
---
a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/MockPipelineManager.java
+++
b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/MockPipelineManager.java
@@ -48,6 +48,7 @@
public class MockPipelineManager implements PipelineManager {
private final PipelineStateManager stateManager;
+ private boolean hasEnoughSpaceBehavior = true;
public MockPipelineManager(DBStore dbStore, SCMHAManager scmhaManager,
NodeManager nodeManager) throws IOException {
@@ -352,6 +353,10 @@ public boolean isPipelineCreationFrozen() {
@Override
public boolean hasEnoughSpace(Pipeline pipeline, long containerSize) {
- return false;
+ return hasEnoughSpaceBehavior;
+ }
+
+ public void setHasEnoughSpaceBehavior(boolean hasEnoughSpace) {
+ hasEnoughSpaceBehavior = hasEnoughSpace;
}
}
```
--
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]
