sumitagrawl commented on code in PR #10074:
URL: https://github.com/apache/ozone/pull/10074#discussion_r3117124561
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManagerImpl.java:
##########
@@ -239,7 +239,8 @@ private void initialize() throws IOException {
final ContainerInfo container = iterator.next();
Objects.requireNonNull(container, "container == null");
containers.addContainer(container);
- if (container.getState() == LifeCycleState.OPEN) {
+ if (container.getState() == LifeCycleState.OPEN
+ && container.getPipelineID() != null) {
Review Comment:
any specific reason for adding this check? Do possible that container open
is reported but pipeline does not exist (closed due to some error), can have
some impact with the check
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManagerImpl.java:
##########
@@ -334,12 +339,16 @@ public void addContainer(final ContainerInfoProto
containerInfo)
transactionBuffer.addToBuffer(containerStore,
containerID, container);
containers.addContainer(container);
- if (pipelineManager.containsPipeline(pipelineID)) {
+ if (pipelineID != null &&
pipelineManager.containsPipeline(pipelineID)) {
pipelineManager.addContainerToPipeline(pipelineID, containerID);
} else if (containerInfo.getState().
equals(LifeCycleState.OPEN)) {
- // Pipeline should exist, but not
- throw new PipelineNotFoundException();
+ if (pipelineID != null) {
Review Comment:
Null pipelineId also represent PipelineNotFound Exception
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -415,8 +415,21 @@ public List<ContainerWithPipeline>
getExistContainerWithPipelinesInBatch(
ContainerWithPipeline cp = getContainerWithPipelineCommon(containerID);
cpList.add(cp);
} catch (IOException ex) {
- //not found , just go ahead
- LOG.error("Container with common pipeline not found: {}", ex);
+ // Pipeline lookup failed (e.g., QUASI_CLOSED container whose pipeline
+ // has already been cleaned up). Return the container metadata without
a
+ // pipeline so that callers (e.g., Recon's sync) can still record the
+ // container rather than losing it silently.
+ LOG.warn("Pipeline lookup failed for container {}; returning container
"
+ + "without pipeline. Cause: {}", containerID, ex.getMessage());
+ try {
+ ContainerInfo info = scm.getContainerManager()
+ .getContainer(ContainerID.valueOf(containerID));
+ cpList.add(new ContainerWithPipeline(info, null));
Review Comment:
are we returning pipeline as null here ?
--
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]
