Re: [PR] HDDS-15135. Import container to another volume if already exists in selected one [ozone]

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

ArafatKhan2198 commented on code in PR #10161:
URL: https://github.com/apache/ozone/pull/10161#discussion_r3186644306


##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/ContainerImporter.java:
##########
@@ -90,53 +94,170 @@ public boolean isAllowedContainerImport(long containerID) {
   public void importContainer(long containerID, Path tarFilePath,
       HddsVolume targetVolume, CopyContainerCompression compression)
       throws IOException {
+    markContainerImportInProgress(containerID, tarFilePath);
+
+    try {
+      checkContainerCanBeImported(containerID);
+      doImportContainer(containerID, tarFilePath, targetVolume, compression);
+    } finally {
+      importContainerProgress.remove(containerID);
+      deleteFileQuietely(tarFilePath);

Review Comment:
   We already have a utility method for it - FileUtils.deleteQuietly 
   could we use it?



##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java:
##########
@@ -260,6 +262,43 @@ protected void createContainerMetaData(File containerMetaDataPath,
     assertEquals(2, callCount.get());
   }
 

Review Comment:
   Suggestion - DO you think the test coverage should verify non-retriable failures are not retried
   
   The PR explicitly says retry is scoped only to CONTAINER_ALREADY_EXISTS. That behavior should have a test.
   
   For example, mock controller.importContainer(...) to throw a normal IOException or a StorageContainerException with a different result code. Then verify only the initially selected volume was attempted.



##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/ContainerImporter.java:
##########
@@ -90,53 +94,170 @@ public boolean isAllowedContainerImport(long containerID) {
   public void importContainer(long containerID, Path tarFilePath,
       HddsVolume targetVolume, CopyContainerCompression compression)
       throws IOException {
+    markContainerImportInProgress(containerID, tarFilePath);
+
+    try {
+      checkContainerCanBeImported(containerID);
+      doImportContainer(containerID, tarFilePath, targetVolume, compression);
+    } finally {
+      importContainerProgress.remove(containerID);
+      deleteFileQuietely(tarFilePath);
+    }
+  }
+
+  public void importContainerWithVolumeRetry(long containerID, Path tarFilePath,

Review Comment:
   We can add JavaDoc to clarify committed-space ownership.
   
   ```
   /**
    * Imports a container and retries on alternate volumes only when the selected
    * volume already has the container directory.
    *
    * The caller is responsible for releasing committed bytes on
    * initialTargetVolume. This method releases committed bytes only for
    * retry-selected volumes.
    */
   ```



##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/replication/TestContainerImporter.java:
##########
@@ -234,6 +237,53 @@ public void testImportContainerResetsLastScanTime() throws Exception {
     assertEquals(Optional.empty(), containerData.lastDataScanTime());
   }
 
+  @Test
+  public void testImportWithVolumeRetrySkipsSelectedVolumeWithContainerDir()

Review Comment:
   The new tests cover the successful retry case, which is good. But I would also add a negative test where every candidate volume already has the stale container directory.
   
   Expected behavior:
   
   Import/create tries all candidate volumes.
   It fails with CONTAINER_ALREADY_EXISTS.
   Committed bytes are restored on all attempted volumes.
   The tarball is deleted.
   No container is added to containerSet.
   
   This is important because the PR changes the final error message to “already exists on all candidate volumes”, but currently the main added tests mostly verify the happy retry path.



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