[GitHub] [ozone] sodonnel commented on a diff in pull request #5324: HDDS-9322. Remove duplicate containers when loading volumes on a datanode

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

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


##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java:
##########
@@ -240,6 +249,80 @@ public void verifyAndFixupContainerData(ContainerData containerData)
     }
   }
 
+  private void resolveDuplicate(KeyValueContainer existing,
+      KeyValueContainer toAdd) throws IOException {
+    if (existing.getContainerData().getReplicaIndex() != 0 ||
+        toAdd.getContainerData().getReplicaIndex() != 0) {
+      // This is an EC container. As EC Containers don't have a BSCID, we can't
+      // know which one has the most recent data. Additionally, it is possible
+      // for both copies to have a different replica index for the same
+      // container. Therefore we just let whatever one is loaded first win AND
+      // leave the other one on disk.
+      LOG.warn("Container {} is present at {} and at {}. Both are EC " +
+              "containers. Leaving both containers on disk.",
+          existing.getContainerData().getContainerID(),
+          existing.getContainerData().getContainerPath(),
+          toAdd.getContainerData().getContainerPath());
+      return;
+    }
+
+    long existingBCSID = existing.getBlockCommitSequenceId();
+    ContainerProtos.ContainerDataProto.State existingState
+        = existing.getContainerState();
+    long toAddBCSID = toAdd.getBlockCommitSequenceId();
+    ContainerProtos.ContainerDataProto.State toAddState
+        = toAdd.getContainerState();
+
+    if (existingState != toAddState) {
+      if (existingState == CLOSED) {
+        // If we have mis-matched states, always pick a closed one
+        LOG.warn("Container {} is present at {} with state CLOSED and at " +
+                "{} with state {}. Removing the latter container.",
+            existing.getContainerData().getContainerID(),
+            existing.getContainerData().getContainerPath(),
+            toAdd.getContainerData().getContainerPath(), toAddState);
+        KeyValueContainerUtil.removeContainer(toAdd.getContainerData(),
+            hddsVolume.getConf());

Review Comment:
   RemoveContainer calls:
   
   ```   
    KeyValueContainerUtil.removeContainerDB(containerData, conf);
    KeyValueContainerUtil.moveToDeletedContainerDir(containerData,
       containerData.getVolume());
   ```
   So it should result in it getting removed from disk.
   
   I believe we should remove them, as otherwise what else is going to clean them up, and there could be a significant number of containers and hence disk space used. 
   
   For there to be 2 copies of a container on a single DN, one of the DNs volumes originally containing the containers must have been marked failed for some time. During that time, a new copy of the container was replicated from another source to bring the redundancy from that failure back up to the expected level. Then, we have had a DN restart bringing the old containers back online.
   So the redundancy should be fine - we have 2 copies available on this DN too, so one should go. What is the value in keeping both? It will just request in queries and confusion about why the DN is consuming more space than it should, and if there are a lot of these containers, nobody is going to be able to go through each manually to make a decision on each of them.



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