HDDS-15596. Hide deprecated CLI options (#10542)

Author: adoroszlai

hadoop-hdds/cli-common/src/main/java/org/apache/hadoop/hdds/cli/DeprecatedCliOption.java

@@ -20,7 +20,7 @@
 import java.io.PrintWriter;
 import java.util.LinkedHashMap;
 import java.util.Map;
-import picocli.CommandLine;
+import java.util.Objects;
 
 /**
  * Emits warnings when deprecated multi-character short CLI options are used.
@@ -51,24 +51,27 @@ private static Map<String, String> buildDeprecatedOptions() {
   }
 
   /**
-   * Print a warning to stderr for each deprecated option present on the command line.
+   * If {@code arg} is a deprecated option (with or without {@code =value} part),
+   * print a warning to stderr and return with the recommended replacement option.
    */
-  public static void warnIfMatched(CommandLine.ParseResult parseResult) {
-    if (parseResult == null) {
-      return;
+  public static String toNonDeprecated(String arg, PrintWriter err) {
+    if (arg == null || arg.isEmpty()) {
+      return arg;
     }
 
-    for (CommandLine cli : parseResult.asCommandLineList()) {
-      CommandLine.ParseResult subcommandResult = cli.getParseResult();
-      if (subcommandResult.matchedOptions().isEmpty()) {
-        continue;
-      }
-      for (Map.Entry<String, String> entry : DEPRECATED_OPTIONS.entrySet()) {
-        if (subcommandResult.hasMatchedOption(entry.getKey())) {
-          warn(cli.getErr(), entry.getKey(), entry.getValue());
-        }
-      }
+    String result = arg;
+    String[] parts = arg.split("=", 2);
+    String opt = parts[0];
+    String optToUse = DEPRECATED_OPTIONS.getOrDefault(opt, opt);
+
+    if (!Objects.equals(opt, optToUse)) {
+      warn(err, opt, optToUse);
+      result = parts.length == 2
+          ? optToUse + '=' + parts[1]
+          : optToUse;
     }
+
+    return result;
   }
 
   private static void warn(PrintWriter err, String deprecated, String replacement) {

hadoop-hdds/cli-common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java

@@ -46,10 +46,6 @@ public abstract class GenericCli implements GenericParentCommand {
 
   private UserGroupInformation user;
 
-  private String configurationPath;
-  private String deprecatedConfigurationPath;
-  private boolean isConfigurationPathAdded = false;
-
   @Option(names = {"--verbose"},
       scope = CommandLine.ScopeType.INHERIT,
       description = "More verbose output. Show the stack trace of the errors.")
@@ -63,22 +59,7 @@ public void setConfigurationOverrides(Map<String, String> configOverrides) {
   @Option(names = {"--conf"},
       description = "Path to custom configuration file.")
   public void setConfigurationPath(String configPath) {
-    configurationPath = configPath;
-  }
-
-  /** For backward compatibility. */
-  @Option(names = {"-conf"}, hidden = true)
-  @Deprecated
-  @SuppressWarnings("DeprecatedIsStillUsed")
-  public void setDeprecatedConfigurationPath(String configPath) {
-    deprecatedConfigurationPath = configPath;
-  }
-
-  private String getConfigurationPath() {
-    if (configurationPath != null) {
-      return configurationPath;
-    }
-    return deprecatedConfigurationPath;
+    config.addResource(new Path(configPath));
   }
 
   public GenericCli() {
@@ -87,16 +68,16 @@ public GenericCli() {
 
   public GenericCli(CommandLine.IFactory factory) {
     cmd = new CommandLine(this, factory);
+    ExtensibleParentCommand.addSubcommands(cmd);
+    cmd.getCommandSpec().preprocessor((args, commandSpec, argSpec, info) -> {
+      args.replaceAll(arg -> DeprecatedCliOption.toNonDeprecated(arg, cmd.getErr()));
+      return false;
+    });
+
     cmd.setExecutionExceptionHandler((ex, commandLine, parseResult) -> {
       printError(ex);
       return EXECUTION_ERROR_EXIT_CODE;
     });
-    cmd.setExecutionStrategy(parseResult -> {
-      DeprecatedCliOption.warnIfMatched(parseResult);
-      return new CommandLine.RunLast().execute(parseResult);
-    });
-
-    ExtensibleParentCommand.addSubcommands(cmd);
   }
 
   public void run(String[] argv) {
@@ -135,11 +116,6 @@ public void printError(Throwable error) {
 
   @Override
   public OzoneConfiguration getOzoneConf() {
-    String path = getConfigurationPath();
-    if (path != null && !isConfigurationPathAdded) {
-      config.addResource(new Path(path));
-      isConfigurationPathAdded = true;
-    }
     return config;
   }
 

hadoop-hdds/cli-common/src/test/java/org/apache/hadoop/hdds/cli/TestGenericCliConfiguration.java

@@ -18,49 +18,44 @@
 package org.apache.hadoop.hdds.cli;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
+import picocli.CommandLine;
 
 /**
  * Tests for {@link GenericCli} configuration option handling.
  */
 public class TestGenericCliConfiguration {
 
+  private static Path deprecatedConf;
+  private static Path preferredConf;
+
   private static final class TestGenericCli extends GenericCli {
   }
 
-  @Test
-  public void nonDeprecatedConfWinsWhenBothAreProvided() throws IOException {
-    Path deprecatedConf = writeConf("deprecated");
-    Path preferredConf = writeConf("preferred");
-
-    TestGenericCli cli = new TestGenericCli();
-    cli.getCmd().parseArgs("-conf", deprecatedConf.toString(), "--conf",
-        preferredConf.toString());
-
-    assertThat(cli.getOzoneConf().get("test.key")).isEqualTo("preferred");
+  @BeforeAll
+  static void setup() throws IOException {
+    deprecatedConf = writeConf("deprecated");
+    preferredConf = writeConf("preferred");
   }
 
   @Test
-  public void nonDeprecatedConfWinsRegardlessOfOrder() throws IOException {
-    Path deprecatedConf = writeConf("deprecated");
-    Path preferredConf = writeConf("preferred");
-
-    TestGenericCli cli = new TestGenericCli();
-    cli.getCmd().parseArgs("--conf", preferredConf.toString(), "-conf",
-        deprecatedConf.toString());
-
-    assertThat(cli.getOzoneConf().get("test.key")).isEqualTo("preferred");
+  void confOptionsAreExclusive() {
+    CommandLine cmd = new TestGenericCli().getCmd();
+    assertThrows(CommandLine.OverwrittenOptionException.class,
+        () -> cmd.parseArgs("-conf", deprecatedConf.toString(), "--conf", preferredConf.toString()));
+    assertThrows(CommandLine.OverwrittenOptionException.class,
+        () -> cmd.parseArgs("--conf", deprecatedConf.toString(), "-conf", preferredConf.toString()));
   }
 
   @Test
-  public void deprecatedConfIsUsedWhenNonDeprecatedIsAbsent() throws IOException {
-    Path deprecatedConf = writeConf("deprecated");
-
+  void deprecatedConfIsUsedWhenNonDeprecatedIsAbsent() {
     TestGenericCli cli = new TestGenericCli();
     cli.getCmd().parseArgs("-conf", deprecatedConf.toString());
 

hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/ScmOption.java

@@ -46,12 +46,6 @@ public class ScmOption extends AbstractMixin {
       "ServiceId of SCM HA Cluster")
   private String scmServiceId;
 
-  /** For backward compatibility. */
-  @CommandLine.Option(names = {"-id"}, hidden = true)
-  @Deprecated
-  @SuppressWarnings("DeprecatedIsStillUsed")
-  private String deprecatedScmServiceId;
-
   public ScmClient createScmClient() throws IOException {
     OzoneConfiguration conf = getOzoneConf();
     checkAndSetSCMAddressArg(conf);
@@ -76,9 +70,8 @@ private void checkAndSetSCMAddressArg(MutableConfigurationSource conf) {
 
     // Use the scm service Id passed from the client.
 
-    String serviceId = getScmServiceId();
-    if (StringUtils.isNotEmpty(serviceId)) {
-      conf.set(ScmConfigKeys.OZONE_SCM_DEFAULT_SERVICE_ID, serviceId);
+    if (StringUtils.isNotEmpty(scmServiceId)) {
+      conf.set(ScmConfigKeys.OZONE_SCM_DEFAULT_SERVICE_ID, scmServiceId);
     } else if (StringUtils.isBlank(HddsUtils.getScmServiceId(conf))) {
       // Scm service id is not passed, and scm service id is not defined in
       // the config, assuming it should be non-HA cluster.
@@ -105,11 +98,4 @@ public SCMSecurityProtocol createScmSecurityClient() {
   public String getScm() {
     return scm;
   }
-
-  public String getScmServiceId() {
-    if (StringUtils.isNotEmpty(scmServiceId)) {
-      return scmServiceId;
-    }
-    return deprecatedScmServiceId;
-  }
 }

hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/pipeline/FilterPipelineOptions.java

@@ -49,26 +49,16 @@ public class FilterPipelineOptions {
       description = "[deprecated] Filter pipelines by factor (e.g. ONE, THREE) (implies RATIS replication type)")
   private ReplicationFactor factor;
 
-  /** For backward compatibility. */
-  @CommandLine.Option(
-      names = {"-ffc"},
-      hidden = true
-  )
-  @Deprecated
-  @SuppressWarnings("DeprecatedIsStillUsed")
-  private ReplicationFactor deprecatedFactor;
-
   Optional<Predicate<? super Pipeline>> getReplicationFilter() {
-    ReplicationFactor effectiveFactor = getFactor();
     boolean hasReplication = !Strings.isNullOrEmpty(replication);
-    boolean hasFactor = effectiveFactor != null;
+    boolean hasFactor = factor != null;
     boolean hasReplicationType = !Strings.isNullOrEmpty(replicationType);
 
     if (hasFactor) {
       if (hasReplication) {
         throw new IllegalArgumentException("Factor and replication are mutually exclusive");
       }
-      ReplicationConfig replicationConfig = RatisReplicationConfig.getInstance(effectiveFactor.toProto());
+      ReplicationConfig replicationConfig = RatisReplicationConfig.getInstance(factor.toProto());
       return Optional.of(p -> replicationConfig.equals(p.getReplicationConfig()));
     }
 
@@ -91,8 +81,4 @@ Optional<Predicate<? super Pipeline>> getReplicationFilter() {
 
     return Optional.empty();
   }
-
-  private ReplicationFactor getFactor() {
-    return factor != null ? factor : deprecatedFactor;
-  }
 }

hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/pipeline/ListPipelinesSubcommand.java

@@ -49,16 +49,6 @@ public class ListPipelinesSubcommand extends ScmSubcommand {
       defaultValue = "")
   private String state;
 
-  /** For backward compatibility. */
-  @CommandLine.Option(
-      names = {"-fst"},
-      hidden = true,
-      defaultValue = ""
-  )
-  @Deprecated
-  @SuppressWarnings("DeprecatedIsStillUsed")
-  private String deprecatedState;
-
   @CommandLine.Option(
       names = {"--json"},
       defaultValue = "false",
@@ -73,10 +63,9 @@ public void execute(ScmClient scmClient) throws IOException {
     if (replicationFilter.isPresent()) {
       stream = stream.filter(replicationFilter.get());
     }
-    String effectiveState = getState();
-    if (!Strings.isNullOrEmpty(effectiveState)) {
+    if (!Strings.isNullOrEmpty(state)) {
       stream = stream.filter(p -> p.getPipelineState().toString()
-          .compareToIgnoreCase(effectiveState) == 0);
+          .compareToIgnoreCase(state) == 0);
     }
 
     if (json) {
@@ -87,11 +76,4 @@ public void execute(ScmClient scmClient) throws IOException {
       stream.forEach(System.out::println);
     }
   }
-
-  private String getState() {
-    if (!Strings.isNullOrEmpty(state)) {
-      return state;
-    }
-    return deprecatedState;
-  }
 }