chihsuan commented on code in PR #10819:
URL: https://github.com/apache/ozone/pull/10819#discussion_r3638136059


##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/volume/TestDatanodeHddsVolumeFailureDetection.java:
##########
@@ -60,176 +67,219 @@
 import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
 import org.apache.hadoop.ozone.container.common.volume.MutableVolumeSet;
 import org.apache.hadoop.ozone.container.common.volume.StorageVolume;
+import 
org.apache.hadoop.ozone.container.common.volume.StorageVolume.VolumeState;
 import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
 import org.apache.hadoop.ozone.container.ozoneimpl.OzoneContainer;
 import org.apache.hadoop.ozone.dn.DatanodeTestUtils;
-import org.junit.jupiter.params.ParameterizedTest;
+import org.apache.ozone.test.GenericTestUtils;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInstance;
+import org.junit.jupiter.api.parallel.Execution;
+import org.junit.jupiter.api.parallel.ExecutionMode;
+import org.junit.jupiter.params.AfterParameterizedClassInvocation;
+import org.junit.jupiter.params.BeforeParameterizedClassInvocation;
+import org.junit.jupiter.params.Parameter;
+import org.junit.jupiter.params.ParameterizedClass;
 import org.junit.jupiter.params.provider.ValueSource;
 
 /**
  * This class tests datanode can detect failed volumes.
  */
+@ParameterizedClass
+@ValueSource(booleans = {true, false})
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
+@Execution(ExecutionMode.SAME_THREAD)
 class TestDatanodeHddsVolumeFailureDetection {
   private static final int KEY_SIZE = 128;
 
-  @ParameterizedTest
-  @ValueSource(booleans = {true, false})
-  void corruptChunkFile(boolean schemaV3) throws Exception {
-    try (MiniOzoneCluster cluster = newCluster(schemaV3)) {
-      try (OzoneClient client = cluster.newClient()) {
-        OzoneBucket bucket = TestDataUtil.createVolumeAndBucket(client);
-
-        // write a file
-        String keyName = UUID.randomUUID().toString();
-        long containerId = createKey(bucket, keyName);
-
-        // corrupt chunk file by rename file->dir
-        HddsDatanodeService dn = cluster.getHddsDatanodes().get(0);
-        OzoneContainer oc = dn.getDatanodeStateMachine().getContainer();
-        MutableVolumeSet volSet = oc.getVolumeSet();
-        StorageVolume vol0 = volSet.getVolumesList().get(0);
-        HddsVolume volume = assertInstanceOf(HddsVolume.class, vol0);
-        Path chunksPath = Paths.get(
-            volume.getStorageDir().getPath(),
-            volume.getClusterID(),
-            Storage.STORAGE_DIR_CURRENT,
-            Storage.CONTAINER_DIR + "0",
-            String.valueOf(containerId),
-            OzoneConsts.STORAGE_DIR_CHUNKS
-        );
-        File[] chunkFiles = chunksPath.toFile().listFiles();
-        assertNotNull(chunkFiles);
-
-        try {
-          for (File chunkFile : chunkFiles) {
-            DatanodeTestUtils.injectDataFileFailure(chunkFile);
-          }
-
-          // simulate bad volume by removing write permission on root dir
-          // refer to HddsVolume.check()
-          DatanodeTestUtils.simulateBadVolume(vol0);
-
-          // read written file to trigger checkVolumeAsync
-          readKeyToTriggerCheckVolumeAsync(bucket, keyName);
-
-          // should trigger checkVolumeAsync and
-          // a failed volume should be detected
-          DatanodeTestUtils.waitForHandleFailedVolume(volSet, 1);
-        } finally {
-          // restore for cleanup
-          DatanodeTestUtils.restoreBadVolume(vol0);
-          for (File chunkFile : chunkFiles) {
-            DatanodeTestUtils.restoreDataFileFromFailure(chunkFile);
-          }
-        }
-      }
+  @Parameter
+  private boolean schemaV3;
+
+  private MiniOzoneCluster cluster;
+
+  @BeforeParameterizedClassInvocation
+  void initCluster() throws Exception {
+    cluster = newCluster(schemaV3);
+  }
+
+  @AfterEach
+  void restoreVolume() throws Exception {
+    MutableVolumeSet volumeSet = cluster.getHddsDatanodes().get(0)
+        .getDatanodeStateMachine().getContainer().getVolumeSet();
+    if (!volumeSet.getVolumesList().isEmpty()) {
+      return;
+    }
+    // A schema V3 volume failure closes the shared DB, which requires a
+    // datanode restart. Schema V2 uses per-container DBs, so restoring the
+    // volume to the active map is sufficient for the next test.
+    if (schemaV3) {
+      cluster.restartHddsDatanode(0, true);
+      return;
     }
+    StorageVolume volume = volumeSet.getFailedVolumesList().get(0);
+    volume.setState(VolumeState.NORMAL);
+    volume.start();
+    volumeSet.setVolumeMapForTesting(

Review Comment:
   Would restarting the V2 datanode also be a simpler way to avoid carrying 
this state into the next test? None of this breaks the current tests, but it is 
a contaminated state that future tests could trip over.
   
   If you prefer to keep the in-place restore for speed, a short comment 
documenting that the volume intentionally stays in `failedVolumeMap` (and why 
that is safe) would help the next reader.
   



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/volume/TestDatanodeHddsVolumeFailureDetection.java:
##########
@@ -240,64 +290,55 @@ void corruptDbFile(boolean schemaV3) throws Exception {
    * As a workaround, we test the helper method directly.
    * As we test the helper method directly, we cannot test for schemas older 
than V3.
    *
-   * @param schemaV3
    * @throws Exception

Review Comment:
   nit: the `@param schemaV3` was removed but the bare `@throws Exception` 
remains. This javadoc could drop it too.



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/volume/TestDatanodeHddsVolumeFailureDetection.java:
##########
@@ -319,6 +367,10 @@ private static MiniOzoneCluster newCluster(boolean 
schemaV3)
     // keep the cache size = 1, so we could trigger io exception on
     // reading on-disk db instance
     ozoneConfig.setInt(OZONE_CONTAINER_CACHE_SIZE, 1);
+    ozoneConfig.setTimeDuration(HDDS_HEARTBEAT_INTERVAL, 100, 
TimeUnit.MILLISECONDS);
+    ozoneConfig.setTimeDuration(OZONE_SCM_HEARTBEAT_PROCESS_INTERVAL, 100, 
TimeUnit.MILLISECONDS);
+    ozoneConfig.setTimeDuration(OZONE_SCM_STALENODE_INTERVAL, 1, 
TimeUnit.SECONDS);
+    ozoneConfig.setTimeDuration(OZONE_SCM_DEADNODE_INTERVAL, 2, 
TimeUnit.SECONDS);

Review Comment:
   Are the 1s stale and 2s dead intervals needed for the restart flow? I wonder 
whether they could be tight on a busy CI runner. If slightly larger values (for 
example stale 3s / dead 6s) still keep the runtime win, they would leave more 
headroom.



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

Reply via email to