[GitHub] [ozone] adoroszlai commented on a diff in pull request #4367: HDDS-8118. Fail container delete on non empty chunks dir

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

adoroszlai commented on code in PR #4367:
URL: https://github.com/apache/ozone/pull/4367#discussion_r1130710694


##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -3607,4 +3607,12 @@
       without using the optimised DAG based pruning approach
     </description>
   </property>
+  <property>
+    <name>ozone.scm.check.empty.container.delete</name>
+    <value>true</value>
+    <tag>DATANODE</tag>

Review Comment:
   `ozone.scm.` prefix doesn't seem to be the best one for datanode-specific setting.



##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1231,19 +1239,41 @@ private void deleteInternal(Container container, boolean force)
         }
         // Safety check that the container is empty.
         // If the container is not empty, it should not be deleted unless the
-        // container is beinf forcefully deleted (which happens when
+        // container is being forcefully deleted (which happens when
         // container is unhealthy or over-replicated).
         if (container.getContainerData().getBlockCount() != 0) {
+          metrics.incContainerDeleteFailedBlockCountNotZero();
           LOG.error("Received container deletion command for container {} but" +
-              " the container is not empty.",
-              container.getContainerData().getContainerID());
+              " the container is not empty with blockCount {}",
+              container.getContainerData().getContainerID(),
+              container.getContainerData().getBlockCount());
           throw new StorageContainerException("Non-force deletion of " +
               "non-empty container is not allowed.",
               DELETE_ON_NON_EMPTY_CONTAINER);
         }
+
+        if (checkIfNoBlockFiles && !container.isEmpty()) {
+          metrics.incContainerDeleteFailedNonEmpty();
+          LOG.error("Received container deletion command for container {} but" +
+                  " the container is not empty",
+              container.getContainerData().getContainerID());
+          throw new StorageContainerException("Non-force deletion of " +
+              "non-empty container:" +
+              container.getContainerData().getContainerID() +
+              " is not allowed.",
+              DELETE_ON_NON_EMPTY_CONTAINER);
+        }
+      } else {
+        metrics.incContainersForceDelete();
       }
       long containerId = container.getContainerData().getContainerID();
       containerSet.removeContainer(containerId);
+    } catch (IOException e) {
+      LOG.error("Could not determine if the container {} is empty",
+          container.getContainerData().getContainerID(), e);
+      throw new StorageContainerException("Could not determine if container "
+          + container.getContainerData().getContainerID() +
+          " is empty", DELETE_ON_NON_EMPTY_CONTAINER);

Review Comment:
   This also catches the `StorageContainerException` (`extends IOException`) thrown above when we know the container has blocks or files, and logs/throws `Could not determine if container ... is empty`.  Will be confusing.



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