errose28 commented on code in PR #11861:
URL: https://github.com/apache/ozone/pull/11861#discussion_r4498368559
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMClientProtocolServer.java:
##########
@@ -257,6 +263,87 @@ private StorageContainerManager
mockStorageContainerManager(List<ContainerInfo>
return storageContainerManager;
}
+ /**
+ * An OPEN container whose pipeline lists a datanode that is not registered
must be served from the live replicas
+ * instead of the stored pipeline, and a pipeline with all nodes registered
is still returned as is.
+ */
+ @Test
+ public void
testGetContainerWithPipelineSkipsStoredPipelineWithUnregisteredNode() throws
Exception {
Review Comment:
These two tests can be parameterized by pipeline state:
```diff
diff --git
a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMClientProtocolServer.java
b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMClientProtocolServer.java
index a4a8fca006..464ebded6d 100644
---
a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMClientProtocolServer.java
+++
b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMClientProtocolServer.java
@@ -79,6 +79,8 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.EnumSource;
/**
* Unit tests to validate the SCMClientProtocolServer
@@ -265,11 +267,15 @@ private StorageContainerManager
mockStorageContainerManager(List<ContainerInfo>
/**
* An OPEN container whose pipeline lists a datanode that is not
registered must be served from the live replicas
- * instead of the stored pipeline, and a pipeline with all nodes
registered is still returned as is.
+ * instead of the stored pipeline, and a pipeline with all nodes
registered is still returned as is. A dormant
+ * pipeline (ozone admin pipeline deactivate) still has open containers
that serve reads, so the check must not
+ * depend on the pipeline state.
*/
- @Test
- public void
testGetContainerWithPipelineSkipsStoredPipelineWithUnregisteredNode() throws
Exception {
- Pipeline storedPipeline = MockPipeline.createPipeline(3);
+ @ParameterizedTest
+ @EnumSource(value = Pipeline.PipelineState.class, names = {"OPEN",
"DORMANT"})
+ public void
testGetContainerWithPipelineSkipsStoredPipelineWithUnregisteredNode(Pipeline.PipelineState
state)
+ throws Exception {
+ Pipeline storedPipeline =
MockPipeline.createPipeline(3).toBuilder().setState(state).build();
Pipeline replicaPipeline = MockPipeline.createPipeline(2);
ContainerInfo container = new ContainerInfo.Builder()
.setContainerID(1)
@@ -304,46 +310,6 @@ public void
testGetContainerWithPipelineSkipsStoredPipelineWithUnregisteredNode(
}
}
- /**
- * A dormant pipeline (ozone admin pipeline deactivate) still has open
containers that serve reads, so the check
- * must not depend on the pipeline state.
- */
- @Test
- public void
testGetContainerWithPipelineSkipsDormantPipelineWithUnregisteredNode() throws
Exception {
- Pipeline dormantPipeline = MockPipeline.createPipeline(3).toBuilder()
- .setState(Pipeline.PipelineState.DORMANT)
- .build();
- Pipeline replicaPipeline = MockPipeline.createPipeline(2);
- ContainerInfo container = new ContainerInfo.Builder()
- .setContainerID(1)
- .setState(LifeCycleState.OPEN)
- .setPipelineID(dormantPipeline.getId())
-
.setReplicationConfig(RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.THREE))
- .build();
-
- ContainerManagerImpl containerManager =
mock(ContainerManagerImpl.class);
-
when(containerManager.getContainer(container.containerID())).thenReturn(container);
- PipelineManager pipelineManager = mock(PipelineManager.class);
-
when(pipelineManager.getPipeline(dormantPipeline.getId())).thenReturn(dormantPipeline);
- when(pipelineManager.createPipelineForRead(any(),
any())).thenReturn(replicaPipeline);
- NodeManager nodeManager = mock(NodeManager.class);
-
when(nodeManager.isNodeRegistered(any(DatanodeDetails.class))).thenReturn(true);
-
when(nodeManager.isNodeRegistered(dormantPipeline.getNodes().get(0))).thenReturn(false);
- StorageContainerManager storageContainerManager =
mockStorageContainerManager(Collections.emptyList());
-
when(storageContainerManager.getContainerManager()).thenReturn(containerManager);
-
when(storageContainerManager.getPipelineManager()).thenReturn(pipelineManager);
-
when(storageContainerManager.getScmNodeManager()).thenReturn(nodeManager);
-
when(storageContainerManager.getScmContext()).thenReturn(SCMContext.emptyContext());
-
- SCMClientProtocolServer testServer = new SCMClientProtocolServer(
- new OzoneConfiguration(), storageContainerManager,
mock(ReconfigurationHandler.class));
- try {
- assertEquals(replicaPipeline,
testServer.getContainerWithPipeline(container.getContainerID()).getPipeline());
- } finally {
- testServer.stop();
- }
- }
-
@Test
public void testLegacyFinalizeScmUpgradeAlreadyFinalized() throws
Exception {
FinalizationManager mockFinalizationManager =
mock(FinalizationManager.class);
```
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerManager.java:
##########
Review Comment:
If Recon already had the pipeline, we may have made an extra RPC call on
line 268 for nothing. We should check Recon's pipeline manager before calling
`getWritePipeline`.
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/scm/TestReconContainerManager.java:
##########
@@ -143,6 +146,60 @@ public void testCheckAndAddNewContainer() throws Exception
{
getContainerManager().getContainer(containerID).getState());
}
+ /**
+ * For an OPEN container SCM can return a pipeline built from reported
replicas instead of the write pipeline.
+ * Recon must record the container against the pipeline in its
ContainerInfo, fetched by ID.
+ */
+ @Test
+ public void testCheckAndAddNewOpenContainerUsesWritePipeline() throws
Exception {
+ ContainerWithPipeline fromScm = getTestContainer(120L,
LifeCycleState.OPEN);
+ Pipeline readPipeline = getRandomPipeline();
+ Pipeline writePipeline = fromScm.getPipeline();
+ ContainerInfo containerInfo = fromScm.getContainerInfo();
+ StorageContainerServiceProvider scmClient =
getContainerManager().getScmClient();
+ when(scmClient.getContainerWithPipeline(containerInfo.getContainerID()))
+ .thenReturn(new ContainerWithPipeline(containerInfo, readPipeline));
+
when(scmClient.getPipeline(containerInfo.getPipelineID().getProtobuf())).thenReturn(writePipeline);
+
+ getContainerManager().checkAndAddNewContainer(containerInfo.containerID(),
OPEN, randomDatanodeDetails());
+
+
assertTrue(getContainerManager().getPipelineToOpenContainer().containsKey(writePipeline.getId()));
+
assertFalse(getContainerManager().getPipelineToOpenContainer().containsKey(readPipeline.getId()));
+ }
+
+ /** The sync helper adds containers through addNewContainer directly, so it
needs the same correction. */
+ @Test
+ public void testAddNewOpenContainerUsesWritePipeline() throws Exception {
+ ContainerWithPipeline fromScm = getTestContainer(122L,
LifeCycleState.OPEN);
Review Comment:
With the previous suggestion to check the pipeline manager before calling
SCM, we would need to update this test. `getTestContainer` already adds the
pipeline to the manager so we would never cover the SCM client fetch.
--
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]
