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