sodonnel commented on code in PR #4684:
URL: https://github.com/apache/ozone/pull/4684#discussion_r1188519750


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/ClosingContainerHandler.java:
##########
@@ -74,6 +79,37 @@ public boolean handle(ContainerCheckRequest request) {
             containerInfo, replica.getDatanodeDetails(), forceClose);
       }
     }
+
+    /*
+     * Empty containers in CLOSING state should be CLOSED.
+     *
+     * These are containers that are allocated in SCM but never got created
+     * on Datanodes. Since these containers don't have any replica associated
+     * with them, they are stuck in CLOSING state forever as there is no
+     * replicas to CLOSE.
+     *
+     * We should wait for sometime before moving the container to CLOSED state.
+     * This will give enough time for Datanodes to report the container,
+     * in cases where the container creation was successful on Datanodes.
+     *
+     * Should we have a separate configuration for this wait time?
+     * For now, we are using ReplicationManagerThread Interval * 5 as the wait
+     * time.
+     */
+
+
+    final Optional<ContainerInfo> emptyContainer = Optional.of(containerInfo)

Review Comment:
   I find this syntax akward to parse in my head. Can we just turn this into a 
simple if statement? I think it results in less lines of code, and is more 
immediately understandable to most people:
   
   ```
   if (c.getReplicationType().equals(ReplicationType.RATIS 
       && request.getContainerReplicas().isEmpty()
       && c.getNumberOfKeys() == 0) {
           LOG.debug("Container {} appears to be empty and have no replicas", 
c.containerID);
           long waitTime = replicationManager.getConfig().getInterval() * 5;
           long closingTime = containerInfo.getStateEnterTime().toEpochMilli();
           if (replicationManager.getClock().getMillis() - closingTime > 
waitTime) {
               LOG.debug("Container {} has been in the CLOSING state with no 
replicas for {}, moving to closed", ...)
               replicationManager.updateContainerState(
                   c.containerID(), LifeCycleEvent.CLOSE));
           }
       }



-- 
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]

Reply via email to