Re: [PR] HDDS-13825. Handle invalid OM DB path and rename FSO repair report fields for clarity [ozone]

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

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


##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/repair/om/TestFSORepairTool.java:
##########
@@ -378,6 +390,39 @@ private int execute(boolean dryRun, String... args) {
         .execute(() -> cmd.execute(argList.toArray(new String[0])));
   }
 
+  @Order(ORDER_DRY_RUN)
+  @Test
+  public void testAlternateOmDbDirName() throws Exception {
+    File original = new File(OMStorage.getOmDbDir(cluster.getConf()), OM_DB_NAME);

Review Comment:
   ```suggestion
       File original = new File(dbPath);
   ```



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/repair/om/TestFSORepairTool.java:
##########
@@ -378,6 +390,39 @@ private int execute(boolean dryRun, String... args) {
         .execute(() -> cmd.execute(argList.toArray(new String[0])));
   }
 
+  @Order(ORDER_DRY_RUN)
+  @Test
+  public void testAlternateOmDbDirName() throws Exception {
+    File original = new File(OMStorage.getOmDbDir(cluster.getConf()), OM_DB_NAME);
+    // Place backup under a different parent directory to ensure we don't
+    // accidentally open the original om.db due to path handling bugs.
+    File backupParent = new File(OMStorage.getOmDbDir(cluster.getConf()), "copy");
+    File backup = new File(backupParent, "om-db-backup");
+
+    if (backup.exists()) {
+      FileUtils.deleteDirectory(backup);
+    }
+    if (backupParent.exists()) {
+      FileUtils.deleteDirectory(backupParent);
+    }
+    boolean created = backupParent.mkdirs();
+    if (!created && !backupParent.exists()) {
+      throw new IOException("Failed to create backup parent directory: " + backupParent);
+    }

Review Comment:
   Please use `@TempDir` instead of manually creating and deleting the extra directory, like this:
   
   ```java
     void testAlternateOmDbDirName(@TempDir File backup)
   ```



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/repair/om/TestFSORepairTool.java:
##########
@@ -378,6 +390,39 @@ private int execute(boolean dryRun, String... args) {
         .execute(() -> cmd.execute(argList.toArray(new String[0])));
   }
 
+  @Order(ORDER_DRY_RUN)
+  @Test
+  public void testAlternateOmDbDirName() throws Exception {

Review Comment:
   Please move this method up to the other dry-run test cases in `TestFSORepairTool.java`, to avoid mixing tests and helpers.



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/repair/om/TestFSORepairTool.java:
##########
@@ -367,6 +368,17 @@ private int dryRun(String... args) {
     return execute(true, args);
   }
 
+  private int executeWithDb(boolean dryRun, String omDbPath, String... args) {
+    List<String> argList = new ArrayList<>(Arrays.asList("om", "fso-tree", "--db", omDbPath));
+    if (dryRun) {
+      argList.add("--dry-run");
+    }
+    argList.addAll(Arrays.asList(args));
+
+    return withTextFromSystemIn("y")
+        .execute(() -> cmd.execute(argList.toArray(new String[0])));
+  }
+
   private int execute(boolean dryRun, String... args) {
     List<String> argList = new ArrayList<>(Arrays.asList("om", "fso-tree", "--db", dbPath));

Review Comment:
   - Please avoid code duplication.  Either make `execute` delegate to this new method, or simply add the new parameter to `execute`.  I prefer the second option, since there are few callers, so the change is not too big.
   - Move `String omDbPath` parameter to be the first, to avoid confusion with `String... args`.



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