adoroszlai commented on code in PR #7368: URL: https://github.com/apache/ozone/pull/7368#discussion_r1870859941
########## hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/repair/om/TestFSORepairTool.java: ########## @@ -0,0 +1,576 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.ozone.repair.om; + +import org.apache.commons.io.IOUtils; +import org.apache.hadoop.fs.CommonConfigurationKeysPublic; +import org.apache.hadoop.fs.FSDataOutputStream; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.contract.ContractTestUtils; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.utils.db.Table; +import org.apache.hadoop.hdds.utils.db.TableIterator; +import org.apache.hadoop.ozone.MiniOzoneCluster; +import org.apache.hadoop.ozone.client.BucketArgs; +import org.apache.hadoop.ozone.client.ObjectStore; +import org.apache.hadoop.ozone.client.OzoneClient; +import org.apache.hadoop.ozone.client.OzoneClientFactory; +import org.apache.hadoop.ozone.client.io.OzoneOutputStream; +import org.apache.hadoop.ozone.om.OMStorage; +import org.apache.hadoop.ozone.om.helpers.BucketLayout; +import org.apache.hadoop.ozone.om.helpers.OmDirectoryInfo; +import org.apache.hadoop.ozone.repair.OzoneRepair; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import picocli.CommandLine; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.io.PrintStream; +import java.nio.charset.StandardCharsets; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.apache.hadoop.ozone.OzoneConsts.OM_DB_NAME; +import static org.apache.hadoop.ozone.OzoneConsts.OZONE_OFS_URI_SCHEME; +import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ADDRESS_KEY; +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * FSORepairTool test cases. + */ +public class TestFSORepairTool { + public static final Logger LOG = LoggerFactory.getLogger(TestFSORepairTool.class); + private final ByteArrayOutputStream out = new ByteArrayOutputStream(); + private final ByteArrayOutputStream err = new ByteArrayOutputStream(); + private static final PrintStream OLD_OUT = System.out; + private static final PrintStream OLD_ERR = System.err; + private static final String DEFAULT_ENCODING = UTF_8.name(); + private MiniOzoneCluster cluster; + private FileSystem fs; + private OzoneClient client; + private OzoneConfiguration conf = null; + + @BeforeEach + public void init() throws Exception { + // Set configs. + conf = new OzoneConfiguration(); + + // Build cluster. + cluster = MiniOzoneCluster.newBuilder(conf).build(); + cluster.waitForClusterToBeReady(); + + // Init ofs. + final String rootPath = String.format("%s://%s/", OZONE_OFS_URI_SCHEME, conf.get(OZONE_OM_ADDRESS_KEY)); + conf.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, rootPath); + fs = FileSystem.get(conf); + client = OzoneClientFactory.getRpcClient(conf); + + System.setOut(new PrintStream(out, false, DEFAULT_ENCODING)); + System.setErr(new PrintStream(err, false, DEFAULT_ENCODING)); + } + + @AfterEach + public void reset() throws IOException { + // reset stream after each unit test + out.reset(); + err.reset(); + + // restore system streams + System.setOut(OLD_OUT); + System.setErr(OLD_ERR); + + if (cluster != null) { + cluster.shutdown(); + } + if (client != null) { + client.close(); + } + IOUtils.closeQuietly(fs); + } + + @Test + public void testConnectedTreeOneBucket() throws Exception { + CommandLine cmd = new CommandLine(new OzoneRepair()).addSubcommand(new CommandLine(new OMRepair()) + .addSubcommand(new FSORepairCLI())); Review Comment: I know this follows existing test pattern, but: ```suggestion CommandLine cmd = new OzoneRepair().getCmd(); ``` This is simpler, and it tests the `ozone repair` command as it exists. By using explicit `addSubcommand` in tests, the CLI could be broken and we wouldn't notice. (This comment applies to all test cases where `new CommandLine` is created.) (I'm making the same change in existing `TestOzoneRepairShell` in #7526.) ########## hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/om/FSORepairCLI.java: ########## @@ -0,0 +1,89 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.repair.om; + +import org.apache.hadoop.hdds.cli.SubcommandWithParent; +import org.kohsuke.MetaInfServices; +import picocli.CommandLine; + +import java.util.concurrent.Callable; + +/** + * Parser for scm.db file. + */ [email protected]( + name = "fso-tree", + description = "Identify and repair a disconnected FSO tree, and mark unreachable entries for deletion. " + + "OM should be stopped while this tool is run." +) +@MetaInfServices(SubcommandWithParent.class) +public class FSORepairCLI implements Callable<Void>, SubcommandWithParent { Review Comment: Please add to parent command explicitly (see suggestion on `OMRepair`). ```suggestion public class FSORepairCLI implements Callable<Void> { ``` ########## hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/om/OMRepair.java: ########## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.repair.om; + +import org.apache.hadoop.hdds.cli.GenericCli; +import org.apache.hadoop.hdds.cli.SubcommandWithParent; +import org.apache.hadoop.ozone.repair.OzoneRepair; +import org.kohsuke.MetaInfServices; +import picocli.CommandLine; + +import java.util.concurrent.Callable; + +/** + * Ozone Repair CLI for OM. + */ [email protected](name = "om", + description = "Operational tool to repair OM.") +@MetaInfServices(SubcommandWithParent.class) +public class OMRepair implements Callable<Void>, SubcommandWithParent { + + @CommandLine.Spec + private CommandLine.Model.CommandSpec spec; + + @CommandLine.ParentCommand + private OzoneRepair parent; Review Comment: Unused, please remove. ########## hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/repair/om/TestFSORepairTool.java: ########## @@ -0,0 +1,576 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.ozone.repair.om; + +import org.apache.commons.io.IOUtils; +import org.apache.hadoop.fs.CommonConfigurationKeysPublic; +import org.apache.hadoop.fs.FSDataOutputStream; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.contract.ContractTestUtils; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.utils.db.Table; +import org.apache.hadoop.hdds.utils.db.TableIterator; +import org.apache.hadoop.ozone.MiniOzoneCluster; +import org.apache.hadoop.ozone.client.BucketArgs; +import org.apache.hadoop.ozone.client.ObjectStore; +import org.apache.hadoop.ozone.client.OzoneClient; +import org.apache.hadoop.ozone.client.OzoneClientFactory; +import org.apache.hadoop.ozone.client.io.OzoneOutputStream; +import org.apache.hadoop.ozone.om.OMStorage; +import org.apache.hadoop.ozone.om.helpers.BucketLayout; +import org.apache.hadoop.ozone.om.helpers.OmDirectoryInfo; +import org.apache.hadoop.ozone.repair.OzoneRepair; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import picocli.CommandLine; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.io.PrintStream; +import java.nio.charset.StandardCharsets; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.apache.hadoop.ozone.OzoneConsts.OM_DB_NAME; +import static org.apache.hadoop.ozone.OzoneConsts.OZONE_OFS_URI_SCHEME; +import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ADDRESS_KEY; +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * FSORepairTool test cases. + */ +public class TestFSORepairTool { + public static final Logger LOG = LoggerFactory.getLogger(TestFSORepairTool.class); + private final ByteArrayOutputStream out = new ByteArrayOutputStream(); + private final ByteArrayOutputStream err = new ByteArrayOutputStream(); + private static final PrintStream OLD_OUT = System.out; + private static final PrintStream OLD_ERR = System.err; + private static final String DEFAULT_ENCODING = UTF_8.name(); + private MiniOzoneCluster cluster; + private FileSystem fs; + private OzoneClient client; + private OzoneConfiguration conf = null; + + @BeforeEach Review Comment: Created HDDS-11863 to speed this test up. ########## hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/om/FSORepairCLI.java: ########## @@ -0,0 +1,89 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.repair.om; + +import org.apache.hadoop.hdds.cli.SubcommandWithParent; +import org.kohsuke.MetaInfServices; +import picocli.CommandLine; + +import java.util.concurrent.Callable; + +/** + * Parser for scm.db file. + */ [email protected]( + name = "fso-tree", + description = "Identify and repair a disconnected FSO tree, and mark unreachable entries for deletion. " + + "OM should be stopped while this tool is run." +) +@MetaInfServices(SubcommandWithParent.class) +public class FSORepairCLI implements Callable<Void>, SubcommandWithParent { + + @CommandLine.ParentCommand + private OMRepair parent; Review Comment: Unused, please remove. ########## hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneRepairShell.java: ########## @@ -160,4 +162,21 @@ public void testQuotaRepair() throws Exception { return false; }, 1000, 10000); } + + @Test + public void testFSORepair() throws Exception { + CommandLine cmd = new CommandLine(new OzoneRepair()).addSubcommand(new CommandLine(new OMRepair()) + .addSubcommand(new FSORepairCLI())); Review Comment: Why do we have this separate test case? Isn't it redundant with one of the cases in `TestFSORepairTool`? ```suggestion CommandLine cmd = new OzoneRepair().getCmd(); ``` ########## hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/om/OMRepair.java: ########## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.repair.om; + +import org.apache.hadoop.hdds.cli.GenericCli; +import org.apache.hadoop.hdds.cli.SubcommandWithParent; +import org.apache.hadoop.ozone.repair.OzoneRepair; +import org.kohsuke.MetaInfServices; +import picocli.CommandLine; + +import java.util.concurrent.Callable; + +/** + * Ozone Repair CLI for OM. + */ [email protected](name = "om", + description = "Operational tool to repair OM.") Review Comment: Please add subcommand explicitly via `@Command` annotation: ```suggestion @CommandLine.Command(name = "om", subcommands = { FSORepairCLI.class, }, description = "Operational tool to repair OM.") ``` ########## hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/om/FSORepairCLI.java: ########## @@ -0,0 +1,89 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.repair.om; + +import org.apache.hadoop.hdds.cli.SubcommandWithParent; +import org.kohsuke.MetaInfServices; +import picocli.CommandLine; + +import java.util.concurrent.Callable; + +/** + * Parser for scm.db file. + */ [email protected]( + name = "fso-tree", + description = "Identify and repair a disconnected FSO tree, and mark unreachable entries for deletion. " + + "OM should be stopped while this tool is run." +) +@MetaInfServices(SubcommandWithParent.class) +public class FSORepairCLI implements Callable<Void>, SubcommandWithParent { + + @CommandLine.ParentCommand + private OMRepair parent; + + @CommandLine.Option(names = {"--db"}, + required = true, + description = "Path to OM RocksDB") + private String dbPath; + + @CommandLine.Option(names = {"-r", "--repair"}, + defaultValue = "false", + description = "Run in repair mode to move unreachable files and directories to deleted tables.") + private boolean repair; + + @CommandLine.Option(names = {"-v", "--volume"}, + description = "Filter by volume name. Add '/' before the volume name.") + private String volume; + + @CommandLine.Option(names = {"-b", "--bucket"}, + description = "Filter by bucket name") + private String bucket; + + @CommandLine.Option(names = {"--verbose"}, + description = "Verbose output. Show all intermediate steps and deleted keys info.") + private boolean verbose; + + @Override + public Void call() throws Exception { + if (repair) { + System.out.println("FSO Repair Tool is running in repair mode"); + } else { + System.out.println("FSO Repair Tool is running in debug mode"); + } + try { + FSORepairTool + repairTool = new FSORepairTool(dbPath, repair, volume, bucket, verbose); + repairTool.run(); + } catch (Exception ex) { + throw new IllegalArgumentException("FSO repair failed: " + ex.getMessage()); + } + + if (verbose) { + System.out.println("FSO repair finished."); + } + + return null; + } + + @Override + public Class<?> getParentType() { + return OMRepair.class; + } Review Comment: ```suggestion ``` ########## hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/om/OMRepair.java: ########## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.repair.om; + +import org.apache.hadoop.hdds.cli.GenericCli; +import org.apache.hadoop.hdds.cli.SubcommandWithParent; +import org.apache.hadoop.ozone.repair.OzoneRepair; +import org.kohsuke.MetaInfServices; +import picocli.CommandLine; + +import java.util.concurrent.Callable; + +/** + * Ozone Repair CLI for OM. + */ [email protected](name = "om", + description = "Operational tool to repair OM.") +@MetaInfServices(SubcommandWithParent.class) +public class OMRepair implements Callable<Void>, SubcommandWithParent { Review Comment: `SubcommandWithParent` implementation should be replaced with `RepairSubcommand` if merged after #7526. If this is merged first, I'll update that PR. -- 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]
