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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
