[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #4207: HDDS-7210. Missing open containers show up as "Closing" on the container report.

["siddhantsangwan (via GitHub)" <gi...@apache.org>]

siddhantsangwan commented on code in PR #4207:
URL: https://github.com/apache/ozone/pull/4207#discussion_r1113903613


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java:
##########
@@ -1613,6 +1614,18 @@ private boolean isOpenContainerHealthy(
         .allMatch(r -> compareState(state, r.getState()));
   }
 
+  private void setHealthStateForClosing(Set<ContainerReplica> replicas,
+                                        ContainerInfo container,
+                                        ReplicationManagerReport report) {
+    if (replicas.size() == 0) {
+      report.incrementAndSample(HealthState.MISSING, container.containerID());
+      report.incrementAndSample(HealthState.UNDER_REPLICATED,
+              container.containerID());
+      report.incrementAndSample(HealthState.MIS_REPLICATED,
+              container.containerID());

Review Comment:
   Yeah, I see that we're also setting the container as mis replicated, under replicated and missing further down in that method for closed containers.
   
   Looks like over time we've diverged from the original intention of the replication manager report:
   
   ```
    * This class is used by ReplicationManager. Each time ReplicationManager runs,
    * it creates a new instance of this class and increments the various counters
    * to allow for creating a report on the various container states within the
    * system. There is a counter for each LifeCycleState (open, closing, closed
    * etc) and the sum of each of the lifecycle state counters should equal the
    * total number of containers in SCM. Ie, each container can only be in one of
    * the Lifecycle states at any time.
   ```
   It specifies that each container should only be in one state at a time. 
   
   I think we need to decide what will best help with debugging. For example, if a container is missing, it's naturally also mis replicated and under replicated. We can choose to count it only once as missing or we can count it in all three categories, but that needs to be done consistently everywhere.
   
   The new RM does not count a missing container as mis replicated, but it does count it as under replicated in `RatisReplicationCheckHandler`. This is because it considers mis replication only when there is no under/over replication.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org