From 4881e2ecae832aa32b91db0248bff2779167ce6c Mon Sep 17 00:00:00 2001 From: "Doroszlai, Attila" Date: Sat, 7 Dec 2024 07:36:37 +0100 Subject: [PATCH] HDDS-11852. Reduce duplication in some GenericCli subclasses --- .../apache/hadoop/hdds/cli/GenericCli.java | 29 +++++++---- .../hadoop/hdds/cli/SubcommandWithParent.java | 51 ------------------- .../apache/hadoop/hdds/cli/OzoneAdmin.java | 36 ------------- .../apache/hadoop/ozone/insight/Insight.java | 4 -- .../hadoop/ozone/TestBlockTokensCLI.java | 4 +- .../ozone/shell/TestNSSummaryAdmin.java | 6 +-- .../ozone/shell/TestOzoneDebugShell.java | 6 +-- .../hadoop/ozone/shell/TestOzoneShellHA.java | 2 +- .../hadoop/ozone/shell/TestReconfigShell.java | 4 +- .../shell/TestTransferLeadershipShell.java | 6 +-- .../apache/hadoop/ozone/debug/OzoneDebug.java | 30 +---------- .../org/apache/hadoop/ozone/freon/Freon.java | 4 -- .../hadoop/ozone/repair/OzoneRepair.java | 30 +---------- .../apache/hadoop/ozone/shell/OzoneRatis.java | 9 ---- .../org/apache/hadoop/ozone/shell/Shell.java | 6 +-- 15 files changed, 36 insertions(+), 191 deletions(-) delete mode 100644 hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/SubcommandWithParent.java diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java index 865b94f514dd..14d454431f97 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java @@ -16,6 +16,7 @@ */ package org.apache.hadoop.hdds.cli; +import java.io.IOException; import java.util.HashMap; import java.util.Map; import java.util.Map.Entry; @@ -25,6 +26,7 @@ import org.apache.hadoop.hdds.conf.OzoneConfiguration; import com.google.common.annotations.VisibleForTesting; +import org.apache.hadoop.security.UserGroupInformation; import picocli.CommandLine; import picocli.CommandLine.ExitCode; import picocli.CommandLine.Model.CommandSpec; @@ -48,25 +50,20 @@ public class GenericCli implements Callable, GenericParentCommand { private String configurationPath; private final CommandLine cmd; + private OzoneConfiguration conf; + private UserGroupInformation user; public GenericCli() { - this(null); + this(CommandLine.defaultFactory()); } - public GenericCli(Class type) { - this(type, CommandLine.defaultFactory()); - } - - public GenericCli(Class type, CommandLine.IFactory factory) { + public GenericCli(CommandLine.IFactory factory) { cmd = new CommandLine(this, factory); cmd.setExecutionExceptionHandler((ex, commandLine, parseResult) -> { printError(ex); return EXECUTION_ERROR_EXIT_CODE; }); - if (type != null) { - SubcommandWithParent.addSubcommands(getCmd(), type); - } ExtensibleParentCommand.addSubcommands(cmd); } @@ -122,6 +119,20 @@ public OzoneConfiguration createOzoneConfiguration() { return ozoneConf; } + public OzoneConfiguration getOzoneConf() { + if (conf == null) { + conf = createOzoneConfiguration(); + } + return conf; + } + + public UserGroupInformation getUser() throws IOException { + if (user == null) { + user = UserGroupInformation.getCurrentUser(); + } + return user; + } + @VisibleForTesting public picocli.CommandLine getCmd() { return cmd; diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/SubcommandWithParent.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/SubcommandWithParent.java deleted file mode 100644 index 398c4f61ba83..000000000000 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/SubcommandWithParent.java +++ /dev/null @@ -1,51 +0,0 @@ -/** - * 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.hdds.cli; - -import picocli.CommandLine; - -import java.util.ServiceLoader; - -/** - * Defines parent command for SPI based subcommand registration. - * @deprecated use more specific interfaces - * @see ExtensibleParentCommand - */ -@Deprecated -public interface SubcommandWithParent { - - static void addSubcommands(CommandLine cli, Class type) { - ServiceLoader registeredSubcommands = - ServiceLoader.load(SubcommandWithParent.class); - for (SubcommandWithParent subcommand : registeredSubcommands) { - if (subcommand.getParentType().equals(type)) { - final CommandLine.Command commandAnnotation = - subcommand.getClass().getAnnotation(CommandLine.Command.class); - CommandLine subcommandCommandLine = new CommandLine(subcommand); - addSubcommands(subcommandCommandLine, subcommand.getClass()); - cli.addSubcommand(commandAnnotation.name(), subcommandCommandLine); - } - } - } - - /** - * Java type of the parent command. - */ - Class getParentType(); - -} diff --git a/hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/cli/OzoneAdmin.java b/hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/cli/OzoneAdmin.java index 5a9736e5e52b..0c182d75e830 100644 --- a/hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/cli/OzoneAdmin.java +++ b/hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/cli/OzoneAdmin.java @@ -17,11 +17,7 @@ */ package org.apache.hadoop.hdds.cli; -import com.google.common.annotations.VisibleForTesting; -import java.io.IOException; -import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.tracing.TracingUtil; -import org.apache.hadoop.security.UserGroupInformation; import picocli.CommandLine; @@ -35,38 +31,6 @@ mixinStandardHelpOptions = true) public class OzoneAdmin extends GenericCli implements ExtensibleParentCommand { - private OzoneConfiguration ozoneConf; - - private UserGroupInformation user; - - public OzoneAdmin() { - super(); - } - - @VisibleForTesting - public OzoneAdmin(OzoneConfiguration conf) { - ozoneConf = conf; - } - - public OzoneConfiguration getOzoneConf() { - if (ozoneConf == null) { - ozoneConf = createOzoneConfiguration(); - } - return ozoneConf; - } - - public UserGroupInformation getUser() throws IOException { - if (user == null) { - user = UserGroupInformation.getCurrentUser(); - } - return user; - } - - /** - * Main for the Ozone Admin shell Command handling. - * - * @param argv - System Args Strings[] - */ public static void main(String[] argv) { new OzoneAdmin().run(argv); } diff --git a/hadoop-ozone/insight/src/main/java/org/apache/hadoop/ozone/insight/Insight.java b/hadoop-ozone/insight/src/main/java/org/apache/hadoop/ozone/insight/Insight.java index b4080796be2a..690783ee411b 100644 --- a/hadoop-ozone/insight/src/main/java/org/apache/hadoop/ozone/insight/Insight.java +++ b/hadoop-ozone/insight/src/main/java/org/apache/hadoop/ozone/insight/Insight.java @@ -34,10 +34,6 @@ mixinStandardHelpOptions = true) public class Insight extends GenericCli { - public Insight() { - super(Insight.class); - } - public static void main(String[] args) throws Exception { new Insight().run(args); } diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestBlockTokensCLI.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestBlockTokensCLI.java index 45897925c9c9..038248945a4f 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestBlockTokensCLI.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestBlockTokensCLI.java @@ -103,7 +103,8 @@ public final class TestBlockTokensCLI { @BeforeAll public static void init() throws Exception { - conf = new OzoneConfiguration(); + ozoneAdmin = new OzoneAdmin(); + conf = ozoneAdmin.getOzoneConf(); conf.set(OZONE_SCM_CLIENT_ADDRESS_KEY, "localhost"); ExitUtils.disableSystemExit(); @@ -117,7 +118,6 @@ public static void init() throws Exception { setSecretKeysConfig(); startCluster(); client = cluster.newClient(); - ozoneAdmin = new OzoneAdmin(conf); } @AfterAll diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestNSSummaryAdmin.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestNSSummaryAdmin.java index fdc3ec000871..65cfb780fbfb 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestNSSummaryAdmin.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestNSSummaryAdmin.java @@ -58,7 +58,8 @@ public class TestNSSummaryAdmin extends StandardOutputTestBase { @BeforeAll public static void init() throws Exception { - conf = new OzoneConfiguration(); + ozoneAdmin = new OzoneAdmin(); + conf = ozoneAdmin.getOzoneConf(); OMRequestTestUtils.configureFSOptimizedPaths(conf, true); conf.set(OZONE_RECON_ADDRESS_KEY, "localhost:9888"); cluster = MiniOzoneCluster.newBuilder(conf) @@ -67,9 +68,6 @@ public static void init() throws Exception { client = cluster.newClient(); store = client.getObjectStore(); - // Client uses server conf for this test - ozoneAdmin = new OzoneAdmin(conf); - volumeName = UUID.randomUUID().toString(); bucketOBS = UUID.randomUUID().toString(); bucketFSO = UUID.randomUUID().toString(); diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneDebugShell.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneDebugShell.java index f4026ed98cbb..d8315cb427dd 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneDebugShell.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneDebugShell.java @@ -83,6 +83,7 @@ public class TestOzoneDebugShell { private static MiniOzoneCluster cluster = null; private static OzoneClient client; + private static OzoneDebug ozoneDebugShell; private static OzoneConfiguration conf = null; @@ -100,7 +101,8 @@ protected static void startCluster() throws Exception { @BeforeAll public static void init() throws Exception { - conf = new OzoneConfiguration(); + ozoneDebugShell = new OzoneDebug(); + conf = ozoneDebugShell.getOzoneConf(); conf.setTimeDuration(OZONE_SCM_HEARTBEAT_PROCESS_INTERVAL, 100, TimeUnit.MILLISECONDS); conf.setTimeDuration(HDDS_HEARTBEAT_INTERVAL, 1, SECONDS); @@ -206,7 +208,6 @@ private int runChunkInfoCommand(String volumeName, String bucketName, getSetConfStringFromConf(OMConfigKeys.OZONE_OM_ADDRESS_KEY), "chunkinfo", bucketPath + Path.SEPARATOR + keyName }; - OzoneDebug ozoneDebugShell = new OzoneDebug(conf); int exitCode = ozoneDebugShell.execute(args); return exitCode; } @@ -218,7 +219,6 @@ private int runChunkInfoAndVerifyPaths(String volumeName, String bucketName, String[] args = new String[] { getSetConfStringFromConf(OMConfigKeys.OZONE_OM_ADDRESS_KEY), "chunkinfo", bucketPath + Path.SEPARATOR + keyName }; - OzoneDebug ozoneDebugShell = new OzoneDebug(conf); int exitCode = 1; try (GenericTestUtils.SystemOutCapturer capture = new GenericTestUtils .SystemOutCapturer()) { diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java index bcca0d51d1dc..4dc06d8eeb91 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java @@ -223,7 +223,7 @@ public static void shutdown() { @BeforeEach public void setup() throws UnsupportedEncodingException { ozoneShell = new OzoneShell(); - ozoneAdminShell = new OzoneAdmin(ozoneConfiguration); + ozoneAdminShell = new OzoneAdmin(); System.setOut(new PrintStream(out, false, DEFAULT_ENCODING)); System.setErr(new PrintStream(err, false, DEFAULT_ENCODING)); } diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestReconfigShell.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestReconfigShell.java index 7c7f2b77ec51..cc508782a3d2 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestReconfigShell.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestReconfigShell.java @@ -67,7 +67,8 @@ public class TestReconfigShell { */ @BeforeAll public static void setup() throws Exception { - OzoneConfiguration conf = new OzoneConfiguration(); + ozoneAdmin = new OzoneAdmin(); + OzoneConfiguration conf = ozoneAdmin.getOzoneConf(); conf.setTimeDuration(OZONE_SCM_STALENODE_INTERVAL, 3, TimeUnit.SECONDS); String omServiceId = UUID.randomUUID().toString(); cluster = MiniOzoneCluster.newHABuilder(conf) @@ -77,7 +78,6 @@ public static void setup() throws Exception { .setNumDatanodes(DATANODE_COUNT) .build(); cluster.waitForClusterToBeReady(); - ozoneAdmin = new OzoneAdmin(cluster.getConf()); ozoneManager = cluster.getOzoneManager(); storageContainerManager = cluster.getStorageContainerManager(); datanodeServices = cluster.getHddsDatanodes(); diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestTransferLeadershipShell.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestTransferLeadershipShell.java index 1f8d42a7f637..cde7583956ce 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestTransferLeadershipShell.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestTransferLeadershipShell.java @@ -52,6 +52,7 @@ public class TestTransferLeadershipShell { private String scmServiceId; private int numOfOMs = 3; private int numOfSCMs = 3; + private OzoneAdmin ozoneAdmin; private static final long SNAPSHOT_THRESHOLD = 5; @@ -62,7 +63,8 @@ public class TestTransferLeadershipShell { */ @BeforeAll public void init() throws Exception { - conf = new OzoneConfiguration(); + ozoneAdmin = new OzoneAdmin(); + conf = ozoneAdmin.getOzoneConf(); omServiceId = "om-service-test1"; scmServiceId = "scm-service-test1"; conf.setLong(ScmConfigKeys.OZONE_SCM_HA_RATIS_SNAPSHOT_THRESHOLD, @@ -95,7 +97,6 @@ public void testOmTransfer() throws Exception { omList.remove(oldLeader); OzoneManager newLeader = omList.get(0); cluster.waitForClusterToBeReady(); - OzoneAdmin ozoneAdmin = new OzoneAdmin(conf); String[] args1 = {"om", "transfer", "-n", newLeader.getOMNodeId()}; ozoneAdmin.execute(args1); Thread.sleep(3000); @@ -119,7 +120,6 @@ public void testScmTransfer() throws Exception { scmList.remove(oldLeader); StorageContainerManager newLeader = scmList.get(0); - OzoneAdmin ozoneAdmin = new OzoneAdmin(conf); String[] args1 = {"scm", "transfer", "-n", newLeader.getScmId()}; ozoneAdmin.execute(args1); cluster.waitForClusterToBeReady(); diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/OzoneDebug.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/OzoneDebug.java index 25ce665cf674..164d07f96b45 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/OzoneDebug.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/OzoneDebug.java @@ -18,13 +18,11 @@ package org.apache.hadoop.ozone.debug; -import com.google.common.annotations.VisibleForTesting; import org.apache.hadoop.hdds.cli.DebugSubcommand; import org.apache.hadoop.hdds.cli.ExtensibleParentCommand; import org.apache.hadoop.hdds.cli.GenericCli; import org.apache.hadoop.hdds.cli.HddsVersionProvider; -import org.apache.hadoop.hdds.conf.OzoneConfiguration; import picocli.CommandLine; /** @@ -36,33 +34,7 @@ mixinStandardHelpOptions = true) public class OzoneDebug extends GenericCli implements ExtensibleParentCommand { - private OzoneConfiguration ozoneConf; - - public OzoneDebug() { - super(OzoneDebug.class); - } - - @VisibleForTesting - public OzoneDebug(OzoneConfiguration configuration) { - super(OzoneDebug.class); - this.ozoneConf = configuration; - } - - public OzoneConfiguration getOzoneConf() { - if (ozoneConf == null) { - ozoneConf = createOzoneConfiguration(); - } - return ozoneConf; - } - - /** - * Main for the Ozone Debug shell Command handling. - * - * @param argv - System Args Strings[] - * @throws Exception - */ - public static void main(String[] argv) throws Exception { - + public static void main(String[] argv) { new OzoneDebug().run(argv); } diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/Freon.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/Freon.java index 877e25ac7ccd..ccae53f345b6 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/Freon.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/Freon.java @@ -84,10 +84,6 @@ public class Freon extends GenericCli { public static final Logger LOG = LoggerFactory.getLogger(Freon.class); - public Freon() { - super(Freon.class); - } - @Option(names = "--server", description = "Enable internal http server to provide metric " + "and profile endpoint") diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/OzoneRepair.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/OzoneRepair.java index 3bc3f2c99df4..b1ed206f975e 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/OzoneRepair.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/OzoneRepair.java @@ -18,12 +18,10 @@ package org.apache.hadoop.ozone.repair; -import com.google.common.annotations.VisibleForTesting; import org.apache.hadoop.hdds.cli.ExtensibleParentCommand; import org.apache.hadoop.hdds.cli.GenericCli; import org.apache.hadoop.hdds.cli.HddsVersionProvider; import org.apache.hadoop.hdds.cli.RepairSubcommand; -import org.apache.hadoop.hdds.conf.OzoneConfiguration; import picocli.CommandLine; import java.nio.charset.StandardCharsets; @@ -42,33 +40,7 @@ public class OzoneRepair extends GenericCli implements ExtensibleParentCommand { "ATTENTION: Running as user %s. Make sure this is the same user used to run the Ozone process." + " Are you sure you want to continue (y/N)? "; - - private OzoneConfiguration ozoneConf; - - public OzoneRepair() { - super(OzoneRepair.class); - } - - @VisibleForTesting - public OzoneRepair(OzoneConfiguration configuration) { - super(OzoneRepair.class); - this.ozoneConf = configuration; - } - - public OzoneConfiguration getOzoneConf() { - if (ozoneConf == null) { - ozoneConf = createOzoneConfiguration(); - } - return ozoneConf; - } - - /** - * Main for the Ozone Repair shell Command handling. - * - * @param argv - System Args Strings[] - * @throws Exception - */ - public static void main(String[] argv) throws Exception { + public static void main(String[] argv) { new OzoneRepair().run(argv); } diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneRatis.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneRatis.java index f471911c1f4e..20f6f683cbfd 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneRatis.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneRatis.java @@ -33,15 +33,6 @@ mixinStandardHelpOptions = true) public class OzoneRatis extends GenericCli { - public OzoneRatis() { - super(OzoneRatis.class); - } - - /** - * Main for the OzoneRatis Command handling. - * - * @param argv - System Args Strings[] - */ public static void main(String[] argv) throws Exception { new OzoneRatis().run(argv); } diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/Shell.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/Shell.java index 44893f78e667..3291ce87b088 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/Shell.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/Shell.java @@ -58,11 +58,7 @@ public abstract class Shell extends GenericCli { private boolean interactive; public Shell() { - this(null); - } - - public Shell(Class type) { - super(type, new PicocliCommandsFactory()); + super(new PicocliCommandsFactory()); } public String name() {