adoroszlai commented on code in PR #10494:
URL: https://github.com/apache/ozone/pull/10494#discussion_r3401997820
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/pipeline/ListPipelinesSubcommand.java:
##########
@@ -63,9 +73,9 @@ public void execute(ScmClient scmClient) throws IOException {
if (replicationFilter.isPresent()) {
stream = stream.filter(replicationFilter.get());
}
- if (!Strings.isNullOrEmpty(state)) {
+ if (!Strings.isNullOrEmpty(getState())) {
stream = stream.filter(p -> p.getPipelineState().toString()
- .compareToIgnoreCase(state) == 0);
+ .compareToIgnoreCase(getState()) == 0);
Review Comment:
nit: please store in local variable `effectiveState` like in some other
classes
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/om/PrepareSubCommand.java:
##########
@@ -169,4 +205,28 @@ private void execute(OzoneManagerProtocol client) throws
Exception {
}
}
+ private long getTxnApplyWaitTimeSeconds() {
+ return deprecatedTxnApplyWaitTimeSeconds != null
+ ? deprecatedTxnApplyWaitTimeSeconds
+ : txnApplyWaitTimeSeconds;
Review Comment:
Since the options are not exclusive, the non-deprecated one should be
preferred.
##########
hadoop-hdds/cli-common/src/main/java/org/apache/hadoop/hdds/cli/DeprecatedCliOption.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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 java.io.PrintWriter;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import picocli.CommandLine;
+
+/**
+ * Emits warnings when deprecated multi-character short CLI options are used.
+ */
+public final class DeprecatedCliOption {
+
+ private static final Map<String, String> DEPRECATED_OPTIONS =
buildDeprecatedOptions();
+
+ private DeprecatedCliOption() {
+ // no instances
+ }
+
+ 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");
+ options.put("-host", "--service-host");
+ options.put("-nodeid", "--nodeid");
+ options.put("-hostname", "--node-host-address");
+ options.put("-al", "--acls or --acl");
+ options.put("-ffc", "--filter-by-factor");
+ options.put("-fst", "--filter-by-state");
+ options.put("-tawt", "--transaction-apply-wait-timeout");
+ options.put("-tact", "--transaction-apply-check-interval");
+ options.put("-pct", "--prepare-check-interval");
+ options.put("-pt", "--prepare-timeout");
+ return options;
+ }
+
+ /**
+ * Print a warning to stderr for each deprecated option present on the
command line.
+ */
+ public static void warnIfMatched(CommandLine.ParseResult parseResult) {
+ if (parseResult == null) {
+ return;
+ }
+ 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());
Review Comment:
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`.
```sh
$ 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.
```suggestion
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());
}
```
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/om/DecommissionOMSubcommand.java:
##########
@@ -67,15 +67,11 @@ public class DecommissionOMSubcommand implements
Callable<Void> {
@CommandLine.Mixin
private OmAddressOptions.MandatoryServiceIdMixin omServiceOption;
- @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;
Review Comment:
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?
##########
hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/cli/TestDeprecatedCliOption.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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 static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import org.apache.hadoop.hdds.scm.cli.pipeline.ListPipelinesSubcommand;
+import org.junit.jupiter.api.Test;
+import picocli.CommandLine;
+
+/**
+ * Tests for deprecated CLI option warnings.
+ */
+public class TestDeprecatedCliOption {
+
+ @Test
+ public void warnsForDeprecatedOption() {
+ ListPipelinesSubcommand cmd = new ListPipelinesSubcommand();
+ 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;
+ });
Review Comment:
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.
##########
hadoop-hdds/cli-common/src/main/java/org/apache/hadoop/hdds/cli/DeprecatedCliOption.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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 java.io.PrintWriter;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import picocli.CommandLine;
+
+/**
+ * Emits warnings when deprecated multi-character short CLI options are used.
+ */
+public final class DeprecatedCliOption {
+
+ private static final Map<String, String> DEPRECATED_OPTIONS =
buildDeprecatedOptions();
+
+ private DeprecatedCliOption() {
+ // no instances
+ }
+
+ 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");
Review Comment:
Let's suggest only one replacement, double option in `''` seems odd. Same
for `-al`.
```
WARNING: Option '-al' is deprecated. Use '--acls or --acl' instead.
```
--
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]
