HDDS-7957. Deprecate multi-char short options by hani-fouladgar · Pull Request #10494 · apache/ozone

hani-fouladgar · 2026-06-11T17:19:00Z

What changes were proposed in this pull request?

This PR deprecates multi-character short CLI options across Ozone PicoCLI tools, following the same pattern already used for -id and -host in OmAddressOptions. Long option names are now primary; old multi-char shorts remain supported but hidden from help. When a deprecated option is used, a warning is printed to stderr.

Deprecate multi-char short options

Multi-char short flags are split from their long equivalents. Long names are shown in help; deprecated shorts are @deprecated, hidden = true, and merged via getters or ArgGroup for backward compatibility.

Deprecation warnings

Added DeprecatedCliOption in hadoop-hdds/cli-common to detect deprecated options after parsing and print: WARNING: Option '-ffc' is deprecated. Use '--filter-by-factor' instead.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7957

How was this patch tested?

Unit tests added

adoroszlai · 2026-06-11T17:25:48Z

Thanks @hani-fouladgar for the patch. Please enable workflows in your fork (but feel free to disable scheduled workflows).

adoroszlai

Thanks @hani-fouladgar for the patch.

adoroszlai · 2026-06-12T08:21:07Z

+    StringWriter err = new StringWriter();
+    CommandLine cli = new CommandLine(cmd);
+    cli.setErr(new PrintWriter(err, true));
+    cli.setExecutionStrategy(parseResult -> {
+      DeprecatedCliOption.warnIfMatched(parseResult);
+      return CommandLine.ExitCode.OK;
+    });


Please extract this to a helper function CommandLine createCommandLine(Object command) to reduce duplication. err will need to be changed to a member variable initialized in @BeforeEach.

This also makes it easier to add few more test cases for other commands.

adoroszlai · 2026-06-12T08:41:31Z

+    PrintWriter err = parseResult.commandSpec().commandLine().getErr();
+    for (Map.Entry<String, String> entry : DEPRECATED_OPTIONS.entrySet()) {
+      if (parseResult.hasMatchedOption(entry.getKey())) {
+        warn(err, entry.getKey(), entry.getValue());


Unfortunately this does not print warnings for subcommands, because parseResult.matchedOptions only contains options of the top-level command.

Example: warning for -conf, but no warning for -ffc.

$ ozone admin -conf etc/hadoop/ozone-site.xml pipeline list -ffc THREE WARNING: Option '-conf' is deprecated. Use '--conf' instead.

The unit test works because the subcommand is used without parents.

Suggested change

PrintWriter err = parseResult.commandSpec().commandLine().getErr();

for (Map.Entry<String, String> entry : DEPRECATED_OPTIONS.entrySet()) {

if (parseResult.hasMatchedOption(entry.getKey())) {

warn(err, entry.getKey(), entry.getValue());

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());

}

adoroszlai · 2026-06-12T08:55:23Z

+  private static Map<String, String> buildDeprecatedOptions() {
+    Map<String, String> options = new LinkedHashMap<>();
+    options.put("-conf", "--conf");
+    options.put("-id", "--service-id or --om-service-id");


Let's suggest only one replacement, double option in '' seems odd. Same for -al.

WARNING: Option '-al' is deprecated. Use '--acls or --acl' instead.

adoroszlai · 2026-06-12T09:00:03Z

-    if (!Strings.isNullOrEmpty(state)) {
+    if (!Strings.isNullOrEmpty(getState())) {
      stream = stream.filter(p -> p.getPipelineState().toString()
-          .compareToIgnoreCase(state) == 0);
+          .compareToIgnoreCase(getState()) == 0);


nit: please store in local variable effectiveState like in some other classes

adoroszlai · 2026-06-12T09:02:49Z

+    return deprecatedTxnApplyWaitTimeSeconds != null
+        ? deprecatedTxnApplyWaitTimeSeconds
+        : txnApplyWaitTimeSeconds;


Since the options are not exclusive, the non-deprecated one should be preferred.

adoroszlai · 2026-06-12T09:16:31Z

-  @CommandLine.Option(names = {"-nodeid", "--nodeid"},
-      description = "NodeID of the OM to be decommissioned.",
-      required = true)
-  private String decommNodeId;
+  @CommandLine.ArgGroup(multiplicity = "1")
+  private NodeIdOptions nodeIdOptions;


How did you decide where to introduce an exclusive ArgGroup and where to add a plain option for the deprecated one? Behavior with ArgGroup matches original one (in that only one of the option names can be used), while two plain options reduce the change both now and when we will remove the deprecated options in a future release.

Should we use one of these styles consistently?

In DecommissionOMSubcommand and DecommissionScmSubcommand, the @Arggroup(multiplicity = "1") is not there just because of deprecation. It marks the parameter as required. Inside that group, --nodeid and -nodeid are just alternate names for the same value, resolved in a getter — the same pattern as OmAddressOptions
ScmOption, GenericCli, PrepareSubCommand, FilterPipelineOptions, etc. use plain options because those parameters are optional

Thanks for the explanation.

adoroszlai · 2026-06-12T19:24:46Z

  public OzoneConfiguration getOzoneConf() {
+    String path = getConfigurationPath();
+    if (path != null) {
+      config.addResource(new Path(path));
+    }
    return config;


getOzoneConf() may be called multiple times, we should add the extra resource only once.

adoroszlai · 2026-06-12T19:30:39Z

+/**
+ * Tests for {@link GenericCli} configuration option handling.
+ */
+public class TestGenericCliConfiguration {


Thanks for adding this test. Ideally it should be in hadoop-hdds/cli-common/src/test/java/org/apache/hadoop/hdds/cli.

9fc7064 moved TestDeprecatedCliOption instead of TestGenericCliConfiguration (as suggested). While TestDeprecatedCliOption also exercises code in hdds-cli-common (DeprecatedCliOption), it used ListPipelinesSubcommand, so I thought we should keep it in ozone-cli-admin. During the move ListPipelinesSubcommand was replaced with TestCommand, which has a simplified option structure: each option has both the deprecated and non-deprecated names. I think that causes the test failure:

Tests run: 3, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.160 s <<< FAILURE! -- in org.apache.hadoop.hdds.cli.TestDeprecatedCliOption org.apache.hadoop.hdds.cli.TestDeprecatedCliOption.doesNotWarnForLongOption -- Time elapsed: 0.012 s <<< FAILURE! java.lang.AssertionError: Expecting empty but was: "WARNING: Option '-ffc' is deprecated. Use '--filter-by-factor' instead. " at org.apache.hadoop.hdds.cli.TestDeprecatedCliOption.doesNotWarnForLongOption(TestDeprecatedCliOption.java:85)

Please restore TestDeprecatedCliOption to its previous state and move TestGenericCliConfiguration instead.

adoroszlai · 2026-06-13T05:13:32Z

+/**
+ * Tests for {@link GenericCli} configuration option handling.
+ */
+public class TestGenericCliConfiguration {


9fc7064 moved TestDeprecatedCliOption instead of TestGenericCliConfiguration (as suggested). While TestDeprecatedCliOption also exercises code in hdds-cli-common (DeprecatedCliOption), it used ListPipelinesSubcommand, so I thought we should keep it in ozone-cli-admin. During the move ListPipelinesSubcommand was replaced with TestCommand, which has a simplified option structure: each option has both the deprecated and non-deprecated names. I think that causes the test failure:

Tests run: 3, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.160 s <<< FAILURE! -- in org.apache.hadoop.hdds.cli.TestDeprecatedCliOption org.apache.hadoop.hdds.cli.TestDeprecatedCliOption.doesNotWarnForLongOption -- Time elapsed: 0.012 s <<< FAILURE! java.lang.AssertionError: Expecting empty but was: "WARNING: Option '-ffc' is deprecated. Use '--filter-by-factor' instead. " at org.apache.hadoop.hdds.cli.TestDeprecatedCliOption.doesNotWarnForLongOption(TestDeprecatedCliOption.java:85)

Please restore TestDeprecatedCliOption to its previous state and move TestGenericCliConfiguration instead.

adoroszlai · 2026-06-13T05:14:47Z

+
+    <!-- Test dependencies -->
+    <dependency>
+      <groupId>org.apache.ozone</groupId>
+      <artifactId>hdds-test-utils</artifactId>
+      <scope>test</scope>
+    </dependency>


This seems to be unnecessary. Neither TestGenericCliConfiguration nor TestDeprecatedCliOption use code from hdds-test-utils.

adoroszlai · 2026-06-14T07:09:31Z

Thanks @hani-fouladgar for the patch.

Deprecate multi-char short options

f617c89

adoroszlai changed the title ~~Deprecate multi-char short options~~ HDDS-7957. Deprecate multi-char short options Jun 11, 2026

hani-fouladgar added 2 commits June 11, 2026 16:15

Trigger CI tests

f0d6da8

Fix build issue.

34089a5

adoroszlai reviewed Jun 12, 2026

View reviewed changes

adoroszlai mentioned this pull request Jun 12, 2026

HDDS-7956. Deprecate camelCase and under_score style long options #10495

Draft

Some minure changes to address reviewer comments.

20d2499

hani-fouladgar requested a review from adoroszlai June 12, 2026 19:15

hani-fouladgar marked this pull request as draft June 12, 2026 19:17

adoroszlai reviewed Jun 12, 2026

View reviewed changes

Some minor changes to address reviewer comments.

9fc7064

adoroszlai reviewed Jun 13, 2026

View reviewed changes

hani-fouladgar added 2 commits June 13, 2026 10:55

Fix the code pass the integ failure.

9432dda

Fix the code pass the integ failure.

91d1a16

adoroszlai marked this pull request as ready for review June 14, 2026 05:26

adoroszlai approved these changes Jun 14, 2026

View reviewed changes

adoroszlai merged commit 175e293 into apache:master Jun 14, 2026
60 of 61 checks passed

-    PrintWriter err = parseResult.commandSpec().commandLine().getErr();
-    for (Map.Entry<String, String> entry : DEPRECATED_OPTIONS.entrySet()) {
-      if (parseResult.hasMatchedOption(entry.getKey())) {
-        warn(err, entry.getKey(), entry.getValue());
+    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());
+        }

Conversation

hani-fouladgar commented Jun 11, 2026

What changes were proposed in this pull request?

Deprecate multi-char short options

Deprecation warnings

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai commented Jun 11, 2026

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adoroszlai commented Jun 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants