rakeshadr commented on code in PR #10326:
URL: https://github.com/apache/ozone/pull/10326#discussion_r3374488983
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerManager.java:
##########
@@ -210,28 +211,33 @@ void transitionOpenToClosing(ContainerID containerID,
ContainerInfo containerInf
pipelineToOpenContainer.put(pipelineID, curCnt - 1);
}
}
- updateContainerState(containerID, FINALIZE); // OPEN → CLOSING
}
/**
- * Check if an OPEN container should move to CLOSING based on a healthy
- * non-OPEN DN replica report.
+ * Check if Recon's container lifecycle state needs the Recon-specific
+ * pre-processing required before SCM's shared report handler processes the
+ * replica.
+ *
+ * <p>Recon only handles OPEN to CLOSING here to keep the per-pipeline open
+ * container count accurate. All other known-container lifecycle transitions
+ * are left to SCM's common ICR/FCR state machine, which is invoked after
this
+ * method by Recon's report handlers.
+ *
+ * @param containerID containerID to check
+ * @param replicaState replica state reported by a DataNode
*/
private void checkContainerStateAndUpdate(ContainerID containerID,
ContainerReplicaProto.State
replicaState)
throws IOException, InvalidStateTransitionException {
ContainerInfo containerInfo = getContainer(containerID);
HddsProtos.LifeCycleState reconState = containerInfo.getState();
- if (reconState != HddsProtos.LifeCycleState.OPEN
- || replicaState == ContainerReplicaProto.State.OPEN
- || !isHealthy(replicaState)) {
- return;
+ if (reconState == HddsProtos.LifeCycleState.OPEN
Review Comment:
Clarification:
IIUC, you have made it more readable and there is no logic change, am I
correct?
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/scm/TestReconIncrementalContainerReportHandler.java:
##########
@@ -155,12 +158,13 @@ public void testProcessICRStateMismatch()
assertTrue(containerManager.containerExist(containerID));
assertEquals(1,
containerManager.getContainerReplicas(containerID).size());
- LifeCycleState expectedState = getContainerStateFromReplicaState(state);
LifeCycleState actualState =
containerManager.getContainer(containerID).getState();
assertEquals(expectedState, actualState,
String.format("Expecting %s in container state for replica state %s",
expectedState, state));
+ verify(containerManager.getScmClient(), never())
+ .getContainerWithPipeline(containerID.getId());
}
Review Comment:
Would you mind adding one more test case.
Note: Generated via Claude llm. Please review it and then include, if it
helps code maintenance.
testProcessICRStateMismatch only tests OPEN containers. Add a test for
CLOSING containers:
```
/**
* Verifies that CLOSING containers advance to CLOSED or QUASI_CLOSED via
* SCM's shared ICR state machine without any SCM lookup from Recon's
* pre-processing layer. Recon's checkContainerStateAndUpdate only handles
* OPEN → CLOSING; the parent IncrementalContainerReportHandler applies the
* CLOSE / QUASI_CLOSE event for CLOSING containers.
*/
@Test
public void testClosingContainerAdvancesViaSCMHandlerWithoutScmLookup()
throws IOException, TimeoutException {
for (State replicaState : Arrays.asList(State.CLOSED,
State.QUASI_CLOSED)) {
ContainerWithPipeline containerWithPipeline = getTestContainer(
200L + replicaState.ordinal(), LifeCycleState.CLOSING);
ContainerID containerID =
containerWithPipeline.getContainerInfo().containerID();
LifeCycleState expectedState =
getContainerStateFromReplicaState(replicaState);
ReconContainerManager containerManager = getContainerManager();
containerManager.addNewContainer(containerWithPipeline);
DatanodeDetails datanodeDetails =
containerWithPipeline.getPipeline().getFirstNode();
NodeManager nodeManagerMock = mock(NodeManager.class);
when(nodeManagerMock.getNode(any(DatanodeID.class)))
.thenReturn(datanodeDetails);
IncrementalContainerReportFromDatanode reportMock =
mock(IncrementalContainerReportFromDatanode.class);
when(reportMock.getDatanodeDetails()).thenReturn(datanodeDetails);
IncrementalContainerReportProto containerReport =
getIncrementalContainerReportProto(containerID, replicaState,
datanodeDetails.getUuidString());
when(reportMock.getReport()).thenReturn(containerReport);
ReconIncrementalContainerReportHandler reconIcr =
new ReconIncrementalContainerReportHandler(nodeManagerMock,
containerManager, SCMContext.emptyContext());
reconIcr.onMessage(reportMock, mock(EventPublisher.class));
assertEquals(expectedState,
containerManager.getContainer(containerID).getState(),
String.format("CLOSING + %s replica should advance to %s via SCM
handler",
replicaState, expectedState));
verify(containerManager.getScmClient(), never())
.getContainerWithPipeline(containerID.getId());
}
}
```
--
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]
