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

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

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


##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java:
##########
@@ -581,6 +581,13 @@ public final class ScmConfigKeys {
 
   public static final String OZONE_AUDIT_LOG_DEBUG_CMD_LIST_SCMAUDIT =
       "ozone.audit.log.debug.cmd.list.scmaudit";
+
+  public static final String OZONE_SCM_CHECK_EMPTY_CONTAINER_ON_DISK_ON_DELETE =

Review Comment:
   Do we need a description for this in `ozone-default.xml`?



##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1231,19 +1240,37 @@ 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) {
           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.isContainerEmpty()) {
+          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);
+        }
       }
       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:
   Does a general error code make sense here, instead of `DELETE_ON_NON_EMPTY_CONTAINER` which indicates a deletion on a non-empty container?



##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/interfaces/Container.java:
##########
@@ -57,6 +57,13 @@ void create(VolumeSet volumeSet, VolumeChoosingPolicy volumeChoosingPolicy,
    */
   void delete() throws StorageContainerException;
 
+  /**
+   * Returns true if container is empty
+   * @return true of container is empty
+   * @throws IOException if was unable to check container status.
+   */
+  boolean isContainerEmpty() throws IOException;

Review Comment:
   nit: maybe just `isEmpty`, it's already in the container context.



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