showuon commented on code in PR #13459:
URL: https://github.com/apache/kafka/pull/13459#discussion_r1170888493
##########
tools/src/main/java/org/apache/kafka/tools/FeatureCommand.java:
##########
@@ -193,8 +181,7 @@ static String levelToString(String feature, short level) {
static void handleDescribe(Admin adminClient) throws ExecutionException,
InterruptedException {
FeatureMetadata featureMetadata =
adminClient.describeFeatures().featureMetadata().get();
- TreeSet<String> featureList = new
java.util.TreeSet<>(featureMetadata.supportedFeatures().keySet());
- featureList.forEach(feature -> {
+ featureMetadata.supportedFeatures().keySet().forEach(feature -> {
Review Comment:
If we remove `TreeSet`, we should sort it by ourselves. Maybe
`featureMetadata.supportedFeatures().keySet().stream().sorted().forEach...` ?
##########
tools/src/main/java/org/apache/kafka/tools/FeatureCommand.java:
##########
@@ -307,25 +294,24 @@ private static void update(String op, Admin admin,
Map<String, FeatureUpdate> up
}
});
- AtomicInteger numFailures = new AtomicInteger();
- errors.keySet().forEach(feature -> {
- short level = updates.get(feature).maxVersionLevel();
- Optional<Throwable> maybeThrowable = errors.get(feature);
+ int numFailures = 0;
+ for (Map.Entry<String, Optional<Throwable>> entry : errors.entrySet())
{
Review Comment:
nit: It seems we only need keys, so maybe `for (String feature :
errors.keySet())` ?
##########
core/src/main/scala/kafka/admin/FeatureCommand.scala:
##########
@@ -17,306 +17,12 @@
package kafka.admin
-import kafka.tools.TerseFailure
-import kafka.utils.Exit
-import net.sourceforge.argparse4j.ArgumentParsers
-import net.sourceforge.argparse4j.impl.Arguments.{append, fileType, store,
storeTrue}
-import net.sourceforge.argparse4j.inf.{ArgumentParserException, Namespace,
Subparsers}
-import net.sourceforge.argparse4j.internal.HelpScreenException
-import org.apache.kafka.clients.CommonClientConfigs
-import org.apache.kafka.clients.admin.FeatureUpdate.UpgradeType
-import org.apache.kafka.clients.admin.{Admin, FeatureUpdate,
UpdateFeaturesOptions}
-import org.apache.kafka.common.utils.Utils
-import org.apache.kafka.server.common.MetadataVersion
-
-import java.io.{File, PrintStream}
-import java.util.Properties
-import scala.concurrent.ExecutionException
-import scala.jdk.CollectionConverters._
-import scala.compat.java8.OptionConverters._
-
+@deprecated(since = "3.5")
object FeatureCommand {
def main(args: Array[String]): Unit = {
- val res = mainNoExit(args, System.out)
- Exit.exit(res)
- }
-
- // This is used for integration tests in order to avoid killing the test
with Exit.exit,
- // and in order to capture the command output.
- def mainNoExit(
- args: Array[String],
- out: PrintStream
- ): Int = {
- val parser = ArgumentParsers.newArgumentParser("kafka-features")
- .defaultHelp(true)
- .description("This tool manages feature flags in Kafka.")
- parser.addArgument("--bootstrap-server")
- .help("A comma-separated list of host:port pairs to use for establishing
the connection to the Kafka cluster.")
- .required(true)
-
- parser.addArgument("--command-config")
- .`type`(fileType())
- .help("Property file containing configs to be passed to Admin Client.")
- val subparsers = parser.addSubparsers().dest("command")
- addDescribeParser(subparsers)
- addUpgradeParser(subparsers)
- addDowngradeParser(subparsers)
- addDisableParser(subparsers)
-
- try {
- val namespace = parser.parseArgs(args)
- val command = namespace.getString("command")
-
- val commandConfig = namespace.get[File]("command_config")
- val props = if (commandConfig != null) {
- if (!commandConfig.exists()) {
- throw new TerseFailure(s"Properties file ${commandConfig.getPath}
does not exists!")
- }
- Utils.loadProps(commandConfig.getPath)
- } else {
- new Properties()
- }
- props.put(CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG,
namespace.getString("bootstrap_server"))
- val admin = Admin.create(props)
- try {
- command match {
- case "describe" => handleDescribe(out, admin)
- case "upgrade" => handleUpgrade(out, namespace, admin)
- case "downgrade" => handleDowngrade(out, namespace, admin)
- case "disable" => handleDisable(out, namespace, admin)
- }
- } finally {
- admin.close()
- }
- 0
- } catch {
- case _: HelpScreenException =>
- 0
Review Comment:
Why don't we need `HelpScreenException` now?
##########
tools/src/main/java/org/apache/kafka/tools/FeatureCommand.java:
##########
@@ -0,0 +1,317 @@
+/*
+ * 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.kafka.tools;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.concurrent.ExecutionException;
+import java.util.stream.Collectors;
+import net.sourceforge.argparse4j.ArgumentParsers;
+import net.sourceforge.argparse4j.impl.Arguments;
+import net.sourceforge.argparse4j.inf.ArgumentParser;
+import net.sourceforge.argparse4j.inf.Namespace;
+import net.sourceforge.argparse4j.inf.Subparser;
+import net.sourceforge.argparse4j.inf.Subparsers;
+import org.apache.kafka.clients.admin.Admin;
+import org.apache.kafka.clients.admin.FeatureMetadata;
+import org.apache.kafka.clients.admin.FeatureUpdate;
+import org.apache.kafka.clients.admin.SupportedVersionRange;
+import org.apache.kafka.clients.admin.UpdateFeaturesOptions;
+import org.apache.kafka.clients.admin.UpdateFeaturesResult;
+import org.apache.kafka.common.utils.Exit;
+import org.apache.kafka.common.utils.Utils;
+import org.apache.kafka.server.common.MetadataVersion;
+
+import static net.sourceforge.argparse4j.impl.Arguments.append;
+import static net.sourceforge.argparse4j.impl.Arguments.store;
+import static net.sourceforge.argparse4j.impl.Arguments.storeTrue;
+
+public class FeatureCommand {
+ public static void main(String... args) {
+ Exit.exit(mainNoExit(args));
+ }
+
+ static int mainNoExit(String... args) {
+ try {
+ execute(args);
+ return 0;
+ } catch (TerseException e) {
+ System.err.println(e.getMessage());
+ return 1;
+ } catch (Throwable e) {
+ System.err.println(e.getMessage());
+ System.err.println(Utils.stackTrace(e));
+ return 1;
+ }
+ }
+
+ static void execute(String... args) throws Exception {
+ ArgumentParser parser = ArgumentParsers
+ .newArgumentParser("kafka-features")
+ .defaultHelp(true)
+ .description("This tool manages feature flags in Kafka.");
+ parser
+ .addArgument("--bootstrap-server")
+ .help("A comma-separated list of host:port pairs to use for
establishing the connection to the Kafka cluster.")
+ .required(true);
+
+ parser
+ .addArgument("--command-config")
+ .type(Arguments.fileType())
+ .help("Property file containing configs to be passed to Admin
Client.");
+ Subparsers subparsers = parser.addSubparsers().dest("command");
+ addDescribeParser(subparsers);
+ addUpgradeParser(subparsers);
+ addDowngradeParser(subparsers);
+ addDisableParser(subparsers);
+
+ Namespace namespace = parser.parseArgsOrFail(args);
+ String command = namespace.getString("command");
+ String configPath = namespace.getString("command_config");
+ Properties properties = (configPath == null) ? new Properties() :
Utils.loadProps(configPath);
+
+ String bootstrapServer = namespace.getString("bootstrap_server");
+ if (bootstrapServer != null) {
+ properties.setProperty("bootstrap.servers", bootstrapServer);
+ }
+ if (properties.getProperty("bootstrap.servers") == null) {
+ throw new TerseException("Please specify --bootstrap-server.");
+ }
+
+ try (Admin adminClient = Admin.create(properties)) {
+ switch (command) {
+ case "describe":
+ handleDescribe(adminClient);
+ break;
+ case "upgrade":
+ handleUpgrade(namespace, adminClient);
+ break;
+ case "downgrade":
+ handleDowngrade(namespace, adminClient);
+ break;
+ case "disable":
+ handleDisable(namespace, adminClient);
+ break;
+ default:
+ throw new TerseException("Unknown command " + command);
+ }
+ }
+ }
+
+ private static void addDescribeParser(Subparsers subparsers) {
+ subparsers.addParser("describe")
+ .help("Describes the current active feature flags.");
+ }
+
+ private static void addUpgradeParser(Subparsers subparsers) {
+ Subparser upgradeParser = subparsers.addParser("upgrade")
+ .help("Upgrade one or more feature flags.");
+ upgradeParser.addArgument("--metadata")
+ .help("The level to which we should upgrade the metadata. For
example, 3.3-IV3.")
+ .action(store());
+ upgradeParser.addArgument("--feature")
+ .help("A feature upgrade we should perform, in feature=level
format. For example: `metadata.version=5`.")
+ .action(append());
+ upgradeParser.addArgument("--dry-run")
+ .help("Validate this upgrade, but do not perform it.")
+ .action(storeTrue());
+
+ }
+
+ private static void addDowngradeParser(Subparsers subparsers) {
+ Subparser downgradeParser = subparsers.addParser("downgrade")
+ .help("Upgrade one or more feature flags.");
+ downgradeParser.addArgument("--metadata")
+ .help("The level to which we should downgrade the metadata.
For example, 3.3-IV0.")
+ .action(store());
+ downgradeParser.addArgument("--feature")
+ .help("A feature downgrade we should perform, in feature=level
format. For example: `metadata.version=5`.")
+ .action(append());
+ downgradeParser.addArgument("--unsafe")
+ .help("Perform this downgrade even if it may irreversibly
destroy metadata.")
+ .action(storeTrue());
+ downgradeParser.addArgument("--dry-run")
+ .help("Validate this downgrade, but do not perform it.")
+ .action(storeTrue());
+ }
+
+ private static void addDisableParser(Subparsers subparsers) {
+ Subparser downgradeParser = subparsers.addParser("disable")
+ .help("Disable one or more feature flags. This is the same as
downgrading the version to zero.");
+ downgradeParser.addArgument("--feature")
+ .help("A feature flag to disable.")
+ .action(append());
+ downgradeParser.addArgument("--unsafe")
+ .help("Disable this feature flag even if it may irreversibly
destroy metadata.")
+ .action(storeTrue());
+ downgradeParser.addArgument("--dry-run")
+ .help("Perform a dry-run of this disable operation.")
+ .action(storeTrue());
+ }
+
+ static String levelToString(String feature, short level) {
+ if (feature.equals(MetadataVersion.FEATURE_NAME)) {
+ try {
+ return MetadataVersion.fromFeatureLevel(level).version();
+ } catch (Throwable e) {
+ throw new RuntimeException("UNKNOWN " + level, e);
+ }
+ }
+ return String.valueOf(level);
+ }
+
+ static void handleDescribe(Admin adminClient) throws ExecutionException,
InterruptedException {
+ FeatureMetadata featureMetadata =
adminClient.describeFeatures().featureMetadata().get();
+ featureMetadata.supportedFeatures().keySet().forEach(feature -> {
+ short finalizedLevel =
(featureMetadata.finalizedFeatures().get(feature) == null) ? 0 :
featureMetadata.finalizedFeatures().get(feature).maxVersionLevel();
+ SupportedVersionRange range =
featureMetadata.supportedFeatures().get(feature);
+ System.out.printf("Feature: %s\tSupportedMinVersion:
%s\tSupportedMaxVersion: %s\tFinalizedVersionLevel: %s\tEpoch: %s%n",
+ feature,
+ levelToString(feature, range.minVersion()),
+ levelToString(feature, range.maxVersion()),
+ levelToString(feature, finalizedLevel),
+ (featureMetadata.finalizedFeaturesEpoch().isPresent()) ?
featureMetadata.finalizedFeaturesEpoch().get().toString() : "-");
+ });
+ }
+
+ static String metadataVersionsToString(MetadataVersion first,
MetadataVersion last) {
+ List<MetadataVersion> versions =
Arrays.asList(MetadataVersion.VERSIONS).subList(first.ordinal(), last.ordinal()
+ 1);
+ return versions.stream()
+ .map(String::valueOf)
+ .collect(Collectors.joining(", "));
+ }
+
+ static void handleUpgrade(Namespace namespace, Admin adminClient) throws
TerseException {
+ handleUpgradeOrDowngrade("upgrade", namespace, adminClient,
FeatureUpdate.UpgradeType.UPGRADE);
+ }
+
+ static FeatureUpdate.UpgradeType downgradeType(Namespace namespace) {
+ Boolean unsafe = namespace.getBoolean("unsafe");
+ if (unsafe == null || !unsafe) {
+ return FeatureUpdate.UpgradeType.SAFE_DOWNGRADE;
+ } else {
+ return FeatureUpdate.UpgradeType.UNSAFE_DOWNGRADE;
+ }
+ }
+
+ static void handleDowngrade(Namespace namespace, Admin adminClient) throws
TerseException {
+ handleUpgradeOrDowngrade("downgrade", namespace, adminClient,
downgradeType(namespace));
+ }
+
+ static String[] parseNameAndLevel(String input) {
+ int equalsIndex = input.indexOf("=");
+ if (equalsIndex < 0) {
+ throw new RuntimeException("Can't parse feature=level string " +
input + ": equals sign not found.");
+ }
+ String name = input.substring(0, equalsIndex).trim();
+ String levelString = input.substring(equalsIndex + 1).trim();
+ try {
+ Short.parseShort(levelString);
+ } catch (Throwable t) {
+ throw new RuntimeException("Can't parse feature=level string " +
input + ": " +
+ "unable to parse " + levelString + " as a short.");
+ }
+ return new String[]{name, levelString};
+ }
+
+ private static void handleUpgradeOrDowngrade(String op, Namespace
namespace, Admin admin, FeatureUpdate.UpgradeType upgradeType) throws
TerseException {
+ Map<String, FeatureUpdate> updates = new HashMap<>();
+ MetadataVersion version;
+ String metadata = namespace.getString("metadata");
+ if (metadata != null) {
+ try {
+ version = MetadataVersion.fromVersionString(metadata);
+ } catch (Throwable e) {
+ throw new TerseException("Unsupported metadata version " +
metadata +
+ ". Supported metadata versions are " +
metadataVersionsToString(
+ MetadataVersion.MINIMUM_BOOTSTRAP_VERSION,
MetadataVersion.latest()));
+ }
+ updates.put(MetadataVersion.FEATURE_NAME, new
FeatureUpdate(version.featureLevel(), upgradeType));
+ }
+
+ List<String> features = namespace.getList("feature");
+ if (features != null) {
+ features.forEach(feature -> {
+ String[] nameAndLevel;
+ nameAndLevel = parseNameAndLevel(feature);
+
+ if (updates.put(nameAndLevel[0], new
FeatureUpdate(Short.parseShort(nameAndLevel[1]), upgradeType)) != null) {
+ throw new RuntimeException("Feature " + nameAndLevel[0] +
" was specified more than once.");
+ }
+ });
+ }
+
+ update(op, admin, updates, namespace.getBoolean("dry_run"));
+ }
+
+ static void handleDisable(Namespace namespace, Admin adminClient) throws
TerseException {
+ FeatureUpdate.UpgradeType upgradeType = downgradeType(namespace);
+ Map<String, FeatureUpdate> updates = new HashMap<>();
+
+ namespace.getList("feature").forEach(feature -> {
Review Comment:
`getList` could return `null`. We should handle it.
##########
tools/src/main/java/org/apache/kafka/tools/FeatureCommand.java:
##########
@@ -0,0 +1,331 @@
+/*
+ * 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.kafka.tools;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.TreeSet;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+import net.sourceforge.argparse4j.ArgumentParsers;
+import net.sourceforge.argparse4j.impl.Arguments;
+import net.sourceforge.argparse4j.inf.ArgumentParser;
+import net.sourceforge.argparse4j.inf.Namespace;
+import net.sourceforge.argparse4j.inf.Subparser;
+import net.sourceforge.argparse4j.inf.Subparsers;
+import org.apache.kafka.clients.admin.Admin;
+import org.apache.kafka.clients.admin.FeatureMetadata;
+import org.apache.kafka.clients.admin.FeatureUpdate;
+import org.apache.kafka.clients.admin.SupportedVersionRange;
+import org.apache.kafka.clients.admin.UpdateFeaturesOptions;
+import org.apache.kafka.clients.admin.UpdateFeaturesResult;
+import org.apache.kafka.common.utils.Exit;
+import org.apache.kafka.common.utils.Utils;
+import org.apache.kafka.server.common.MetadataVersion;
+
+import static net.sourceforge.argparse4j.impl.Arguments.append;
+import static net.sourceforge.argparse4j.impl.Arguments.store;
+import static net.sourceforge.argparse4j.impl.Arguments.storeTrue;
+
+public class FeatureCommand {
+ public static void main(String... args) {
+ Exit.exit(mainNoExit(args));
+ }
+
+ static int mainNoExit(String... args) {
+ try {
+ execute(args);
+ return 0;
+ } catch (TerseException e) {
+ System.err.println(e.getMessage());
+ return 1;
+ } catch (Throwable e) {
+ System.err.println(e.getMessage());
+ System.err.println(Utils.stackTrace(e));
+ return 1;
+ }
+ }
+
+ static void execute(String... args) throws Exception {
+ ArgumentParser parser = ArgumentParsers
+ .newArgumentParser("kafka-features")
+ .defaultHelp(true)
+ .description("This tool manages feature flags in Kafka.");
+ parser
+ .addArgument("--bootstrap-server")
+ .help("A comma-separated list of host:port pairs to use for
establishing the connection to the Kafka cluster.")
+ .required(true);
+
+ parser
+ .addArgument("--command-config")
+ .type(Arguments.fileType())
+ .help("Property file containing configs to be passed to Admin
Client.");
+ Subparsers subparsers = parser.addSubparsers().dest("command");
+ addDescribeParser(subparsers);
+ addUpgradeParser(subparsers);
+ addDowngradeParser(subparsers);
+ addDisableParser(subparsers);
+
+ Namespace namespace = parser.parseArgsOrFail(args);
+ String command = namespace.getString("command");
+ String configPath = namespace.getString("command_config");
+ Properties properties = (configPath == null) ? new Properties() :
Utils.loadProps(configPath);
+
+ String bootstrapServer = namespace.getString("bootstrap_server");
+ if (bootstrapServer != null) {
+ properties.setProperty("bootstrap.servers", bootstrapServer);
+ }
+ if (properties.getProperty("bootstrap.servers") == null) {
+ throw new TerseException("Please specify --bootstrap-server.");
+ }
+
+ switch (command) {
+ case "describe": {
+ try (Admin adminClient = Admin.create(properties)) {
+ handleDescribe(adminClient);
+ }
+ break;
+ }
+ case "upgrade": {
+ try (Admin adminClient = Admin.create(properties)) {
+ handleUpgrade(namespace, adminClient);
+ }
+ break;
+ }
+ case "downgrade": {
+ try (Admin adminClient = Admin.create(properties)) {
+ handleDowngrade(namespace, adminClient);
+ }
+ break;
+ }
+ case "disable": {
+ try (Admin adminClient = Admin.create(properties)) {
+ handleDisable(namespace, adminClient);
+ }
+ break;
+ }
+ default:
+ throw new TerseException("Unknown command " + command);
+ }
+ }
+
+ private static void addDescribeParser(Subparsers subparsers) {
+ subparsers.addParser("describe")
+ .help("Describes the current active feature flags.");
+ }
+
+ private static void addUpgradeParser(Subparsers subparsers) {
+ Subparser upgradeParser = subparsers.addParser("upgrade")
+ .help("Upgrade one or more feature flags.");
+ upgradeParser.addArgument("--metadata")
+ .help("The level to which we should upgrade the metadata. For
example, 3.3-IV3.")
+ .action(store());
+ upgradeParser.addArgument("--feature")
+ .help("A feature upgrade we should perform, in feature=level
format. For example: `metadata.version=5`.")
+ .action(append());
+ upgradeParser.addArgument("--dry-run")
+ .help("Validate this upgrade, but do not perform it.")
+ .action(storeTrue());
+
+ }
+
+ private static void addDowngradeParser(Subparsers subparsers) {
+ Subparser downgradeParser = subparsers.addParser("downgrade")
+ .help("Upgrade one or more feature flags.");
+ downgradeParser.addArgument("--metadata")
+ .help("The level to which we should downgrade the metadata.
For example, 3.3-IV0.")
+ .action(store());
+ downgradeParser.addArgument("--feature")
+ .help("A feature downgrade we should perform, in feature=level
format. For example: `metadata.version=5`.")
+ .action(append());
+ downgradeParser.addArgument("--unsafe")
+ .help("Perform this downgrade even if it may irreversibly
destroy metadata.")
+ .action(storeTrue());
+ downgradeParser.addArgument("--dry-run")
+ .help("Validate this downgrade, but do not perform it.")
+ .action(storeTrue());
+ }
+
+ private static void addDisableParser(Subparsers subparsers) {
+ Subparser downgradeParser = subparsers.addParser("disable")
+ .help("Disable one or more feature flags. This is the same as
downgrading the version to zero.");
+ downgradeParser.addArgument("--feature")
+ .help("A feature flag to disable.")
+ .action(append());
+ downgradeParser.addArgument("--unsafe")
+ .help("Disable this feature flag even if it may irreversibly
destroy metadata.")
+ .action(storeTrue());
+ downgradeParser.addArgument("--dry-run")
+ .help("Perform a dry-run of this disable operation.")
+ .action(storeTrue());
+ }
+
+ static String levelToString(String feature, short level) {
+ if (feature.equals(MetadataVersion.FEATURE_NAME)) {
+ try {
+ return MetadataVersion.fromFeatureLevel(level).version();
+ } catch (Throwable e) {
+ throw new RuntimeException("UNKNOWN " + level, e);
Review Comment:
Last time, I just saw the scala code, it didn't include the original error
message. But now, I checked again, it doesn't even throw an exception. So we
should update it.
```
if (feature.equals(MetadataVersion.FEATURE_NAME))
try MetadataVersion.fromFeatureLevel(level).version
catch { case _: Throwable => s"UNKNOWN [$level]"}
```
##########
tools/src/main/java/org/apache/kafka/tools/FeatureCommand.java:
##########
@@ -0,0 +1,317 @@
+/*
+ * 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.kafka.tools;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.concurrent.ExecutionException;
+import java.util.stream.Collectors;
+import net.sourceforge.argparse4j.ArgumentParsers;
+import net.sourceforge.argparse4j.impl.Arguments;
+import net.sourceforge.argparse4j.inf.ArgumentParser;
+import net.sourceforge.argparse4j.inf.Namespace;
+import net.sourceforge.argparse4j.inf.Subparser;
+import net.sourceforge.argparse4j.inf.Subparsers;
+import org.apache.kafka.clients.admin.Admin;
+import org.apache.kafka.clients.admin.FeatureMetadata;
+import org.apache.kafka.clients.admin.FeatureUpdate;
+import org.apache.kafka.clients.admin.SupportedVersionRange;
+import org.apache.kafka.clients.admin.UpdateFeaturesOptions;
+import org.apache.kafka.clients.admin.UpdateFeaturesResult;
+import org.apache.kafka.common.utils.Exit;
+import org.apache.kafka.common.utils.Utils;
+import org.apache.kafka.server.common.MetadataVersion;
+
+import static net.sourceforge.argparse4j.impl.Arguments.append;
+import static net.sourceforge.argparse4j.impl.Arguments.store;
+import static net.sourceforge.argparse4j.impl.Arguments.storeTrue;
+
+public class FeatureCommand {
+ public static void main(String... args) {
+ Exit.exit(mainNoExit(args));
+ }
+
+ static int mainNoExit(String... args) {
+ try {
+ execute(args);
+ return 0;
+ } catch (TerseException e) {
+ System.err.println(e.getMessage());
+ return 1;
+ } catch (Throwable e) {
+ System.err.println(e.getMessage());
+ System.err.println(Utils.stackTrace(e));
Review Comment:
Previously, we don't care about the stack trace, why do we need it now?
##########
tools/src/main/java/org/apache/kafka/tools/FeatureCommand.java:
##########
@@ -0,0 +1,317 @@
+/*
+ * 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.kafka.tools;
+
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.concurrent.ExecutionException;
+import java.util.stream.Collectors;
+import net.sourceforge.argparse4j.ArgumentParsers;
+import net.sourceforge.argparse4j.impl.Arguments;
+import net.sourceforge.argparse4j.inf.ArgumentParser;
+import net.sourceforge.argparse4j.inf.Namespace;
+import net.sourceforge.argparse4j.inf.Subparser;
+import net.sourceforge.argparse4j.inf.Subparsers;
+import org.apache.kafka.clients.admin.Admin;
+import org.apache.kafka.clients.admin.FeatureMetadata;
+import org.apache.kafka.clients.admin.FeatureUpdate;
+import org.apache.kafka.clients.admin.SupportedVersionRange;
+import org.apache.kafka.clients.admin.UpdateFeaturesOptions;
+import org.apache.kafka.clients.admin.UpdateFeaturesResult;
+import org.apache.kafka.common.utils.Exit;
+import org.apache.kafka.common.utils.Utils;
+import org.apache.kafka.server.common.MetadataVersion;
+
+import static net.sourceforge.argparse4j.impl.Arguments.append;
+import static net.sourceforge.argparse4j.impl.Arguments.store;
+import static net.sourceforge.argparse4j.impl.Arguments.storeTrue;
+
+public class FeatureCommand {
+ public static void main(String... args) {
+ Exit.exit(mainNoExit(args));
+ }
+
+ static int mainNoExit(String... args) {
+ try {
+ execute(args);
+ return 0;
+ } catch (TerseException e) {
+ System.err.println(e.getMessage());
+ return 1;
+ } catch (Throwable e) {
Review Comment:
Should we catch `ArgumentParserException` like before?
##########
tools/src/main/java/org/apache/kafka/tools/FeatureCommand.java:
##########
@@ -307,25 +294,24 @@ private static void update(String op, Admin admin,
Map<String, FeatureUpdate> up
}
});
- AtomicInteger numFailures = new AtomicInteger();
- errors.keySet().forEach(feature -> {
- short level = updates.get(feature).maxVersionLevel();
- Optional<Throwable> maybeThrowable = errors.get(feature);
+ int numFailures = 0;
+ for (Map.Entry<String, Optional<Throwable>> entry : errors.entrySet())
{
Review Comment:
Also, in scala code, we traverse with sorted keys. Any reason we don't do it
here?
##########
tools/src/test/java/org/apache/kafka/tools/FeatureCommandTest.java:
##########
@@ -0,0 +1,292 @@
+/*
+ * 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.kafka.tools;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import kafka.test.ClusterInstance;
+import kafka.test.annotation.ClusterTest;
+import kafka.test.annotation.ClusterTestDefaults;
+import kafka.test.annotation.Type;
+import kafka.test.junit.ClusterTestExtensions;
+import net.sourceforge.argparse4j.inf.Namespace;
+import org.apache.kafka.clients.admin.MockAdminClient;
+import org.apache.kafka.server.common.MetadataVersion;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import static java.lang.String.format;
+import static java.util.Collections.emptyMap;
+import static java.util.Collections.singletonMap;
+
+import static
org.apache.kafka.clients.admin.FeatureUpdate.UpgradeType.SAFE_DOWNGRADE;
+import static
org.apache.kafka.clients.admin.FeatureUpdate.UpgradeType.UNSAFE_DOWNGRADE;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+@ExtendWith(value = ClusterTestExtensions.class)
+@ClusterTestDefaults(clusterType = Type.KRAFT)
+@Tag("integration")
+public class FeatureCommandTest {
+
+ private final ClusterInstance cluster;
+ public FeatureCommandTest(ClusterInstance cluster) {
+ this.cluster = cluster;
+ }
+
+ @ClusterTest(clusterType = Type.ZK, metadataVersion =
MetadataVersion.IBP_3_3_IV1)
+ public void testDescribeWithZK() {
+ String commandOutput = ToolsTestUtils.captureStandardOut(() ->
+ assertEquals(0,
FeatureCommand.mainNoExit("--bootstrap-server", cluster.bootstrapServers(),
"describe"))
+ );
+ assertEquals("", commandOutput);
+ }
+
+ @ClusterTest(clusterType = Type.KRAFT, metadataVersion =
MetadataVersion.IBP_3_3_IV1)
+ public void testDescribeWithKRaft() {
+ String commandOutput = ToolsTestUtils.captureStandardOut(() ->
+ assertEquals(0,
FeatureCommand.mainNoExit("--bootstrap-server", cluster.bootstrapServers(),
"describe"))
+ );
+ assertEquals("Feature: metadata.version\tSupportedMinVersion:
3.0-IV1\t" +
+ "SupportedMaxVersion: 3.5-IV1\tFinalizedVersionLevel:
3.3-IV1\t", outputWithoutEpoch(commandOutput));
+ }
+
+ @ClusterTest(clusterType = Type.ZK, metadataVersion =
MetadataVersion.IBP_3_3_IV1)
+ public void testUpgradeMetadataVersionWithZk() {
+ String commandOutput = ToolsTestUtils.captureStandardOut(() ->
+ assertEquals(1,
FeatureCommand.mainNoExit("--bootstrap-server", cluster.bootstrapServers(),
+ "upgrade", "--metadata", "3.3-IV2"))
+ );
+ assertEquals("Could not upgrade metadata.version to 6. Could not apply
finalized feature " +
+ "update because the provided feature is not supported.",
commandOutput);
+ }
+
+ @ClusterTest(clusterType = Type.KRAFT, metadataVersion =
MetadataVersion.IBP_3_3_IV1)
+ public void testUpgradeMetadataVersionWithKraft() {
+ String commandOutput = ToolsTestUtils.captureStandardOut(() ->
+ assertEquals(0,
FeatureCommand.mainNoExit("--bootstrap-server", cluster.bootstrapServers(),
+ "upgrade", "--feature", "metadata.version=5"))
+ );
+ assertEquals("metadata.version was upgraded to 5.", commandOutput);
+
+ commandOutput = ToolsTestUtils.captureStandardOut(() ->
+ assertEquals(0,
FeatureCommand.mainNoExit("--bootstrap-server", cluster.bootstrapServers(),
+ "upgrade", "--metadata", "3.3-IV2"))
+ );
+ assertEquals("metadata.version was upgraded to 6.", commandOutput);
+ }
+
+ @ClusterTest(clusterType = Type.ZK, metadataVersion =
MetadataVersion.IBP_3_3_IV1)
+ public void testDowngradeMetadataVersionWithZk() {
+ String commandOutput = ToolsTestUtils.captureStandardOut(() ->
+ assertEquals(1,
FeatureCommand.mainNoExit("--bootstrap-server", cluster.bootstrapServers(),
+ "disable", "--feature", "metadata.version"))
+ );
+ assertEquals("Could not disable metadata.version. Can not delete
non-existing finalized feature.", commandOutput);
+
+ commandOutput = ToolsTestUtils.captureStandardOut(() ->
+ assertEquals(1,
FeatureCommand.mainNoExit("--bootstrap-server", cluster.bootstrapServers(),
+ "downgrade", "--metadata", "3.3-IV0"))
+ );
+ assertEquals("Could not downgrade metadata.version to 4. Could not
apply finalized feature " +
+ "update because the provided feature is not
supported.", commandOutput);
+
+ commandOutput = ToolsTestUtils.captureStandardOut(() ->
+ assertEquals(1,
FeatureCommand.mainNoExit("--bootstrap-server", cluster.bootstrapServers(),
+ "downgrade", "--unsafe", "--metadata", "3.3-IV0"))
+ );
+ assertEquals("Could not downgrade metadata.version to 4. Could not
apply finalized feature " +
+ "update because the provided feature is not supported.",
commandOutput);
+ }
+
+ @ClusterTest(clusterType = Type.KRAFT, metadataVersion =
MetadataVersion.IBP_3_3_IV1)
+ public void testDowngradeMetadataVersionWithKRaft() {
+ String commandOutput = ToolsTestUtils.captureStandardOut(() ->
+ assertEquals(1,
FeatureCommand.mainNoExit("--bootstrap-server", cluster.bootstrapServers(),
+ "disable", "--feature", "metadata.version"))
+ );
+ assertEquals("Could not disable metadata.version. Invalid update
version 0 for feature " +
+ "metadata.version. Local controller 3000 only supports
versions 1-10", commandOutput);
+
+ commandOutput = ToolsTestUtils.captureStandardOut(() ->
+ assertEquals(1,
FeatureCommand.mainNoExit("--bootstrap-server", cluster.bootstrapServers(),
+ "downgrade", "--metadata", "3.3-IV0"))
+
+ );
+ assertEquals("Could not downgrade metadata.version to 4. Invalid
metadata.version 4. " +
+ "Refusing to perform the requested downgrade because it might
delete metadata information. " +
+ "Retry using UNSAFE_DOWNGRADE if you want to force the
downgrade to proceed.", commandOutput);
+
+ commandOutput = ToolsTestUtils.captureStandardOut(() ->
+ assertEquals(1,
FeatureCommand.mainNoExit("--bootstrap-server", cluster.bootstrapServers(),
+ "downgrade", "--unsafe", "--metadata", "3.3-IV0"))
+
+ );
+ assertEquals("Could not downgrade metadata.version to 4. Invalid
metadata.version 4. " +
+ "Unsafe metadata downgrade is not supported in this version.",
commandOutput);
+ }
+
+ private String outputWithoutEpoch(String output) {
+ int pos = output.indexOf("Epoch: ");
+ return (pos > 0) ? output.substring(0, pos) : output;
+ }
+}
+
+class FeatureCommandUnitTest {
+ @Test
+ public void testLevelToString() {
+ assertEquals("5", FeatureCommand.levelToString("foo.bar", (short) 5));
+ assertEquals("3.3-IV0",
+ FeatureCommand.levelToString(MetadataVersion.FEATURE_NAME,
MetadataVersion.IBP_3_3_IV0.featureLevel()));
+ }
+
+ @Test
+ public void testMetadataVersionsToString() {
+ assertEquals("3.3-IV0, 3.3-IV1, 3.3-IV2, 3.3-IV3",
+
FeatureCommand.metadataVersionsToString(MetadataVersion.IBP_3_3_IV0,
MetadataVersion.IBP_3_3_IV3));
+ }
+
+ @Test
+ public void testdowngradeType() {
+ assertEquals(SAFE_DOWNGRADE, FeatureCommand.downgradeType(
+ new Namespace(singletonMap("unsafe", Boolean.FALSE))));
+ assertEquals(UNSAFE_DOWNGRADE, FeatureCommand.downgradeType(
+ new Namespace(singletonMap("unsafe", Boolean.TRUE))));
+ assertEquals(SAFE_DOWNGRADE, FeatureCommand.downgradeType(new
Namespace(emptyMap())));
+ }
+
+ @Test
+ public void testParseNameAndLevel() {
+ assertArrayEquals(new String[]{"foo.bar", "5"},
FeatureCommand.parseNameAndLevel("foo.bar=5"));
+ assertArrayEquals(new String[]{"quux", "0"},
FeatureCommand.parseNameAndLevel("quux=0"));
+ assertTrue(assertThrows(RuntimeException.class, () ->
FeatureCommand.parseNameAndLevel("baaz"))
+ .getMessage().contains("Can't parse feature=level string baaz:
equals sign not found."));
+ assertTrue(assertThrows(RuntimeException.class, () ->
FeatureCommand.parseNameAndLevel("w=tf"))
+ .getMessage().contains("Can't parse feature=level string w=tf:
unable to parse tf as a short."));
+ }
+
+ private static MockAdminClient buildAdminClient() {
+ Map<String, Short> minSupportedFeatureLevels = new HashMap<>();
+ minSupportedFeatureLevels.put(MetadataVersion.FEATURE_NAME,
MetadataVersion.IBP_3_3_IV0.featureLevel());
+ minSupportedFeatureLevels.put("foo.bar", (short) 0);
+
+ Map<String, Short> featureLevels = new HashMap<>();
+ featureLevels.put(MetadataVersion.FEATURE_NAME,
MetadataVersion.IBP_3_3_IV2.featureLevel());
+ featureLevels.put("foo.bar", (short) 5);
+
+ Map<String, Short> maxSupportedFeatureLevels = new HashMap<>();
+ maxSupportedFeatureLevels.put(MetadataVersion.FEATURE_NAME,
MetadataVersion.IBP_3_3_IV3.featureLevel());
+ maxSupportedFeatureLevels.put("foo.bar", (short) 10);
+
+ return new MockAdminClient.Builder().
+ minSupportedFeatureLevels(minSupportedFeatureLevels).
+ featureLevels(featureLevels).
+ maxSupportedFeatureLevels(maxSupportedFeatureLevels).build();
+ }
+
+ @Test
+ public void testHandleDescribe() {
+ String describeResult = ToolsTestUtils.captureStandardOut(() -> {
+ try {
+ FeatureCommand.handleDescribe(buildAdminClient());
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ });
+ assertEquals(format("Feature: foo.bar\tSupportedMinVersion:
0\tSupportedMaxVersion: 10\tFinalizedVersionLevel: 5\tEpoch: 123%n" +
+ "Feature: metadata.version\tSupportedMinVersion:
3.3-IV0\tSupportedMaxVersion: 3.3-IV3\tFinalizedVersionLevel: 3.3-IV2\tEpoch:
123"), describeResult);
+ }
+
+ @Test
+ public void testHandleUpgrade() {
+ Map<String, Object> namespace = new HashMap<>();
+ namespace.put("metadata", "3.3-IV1");
+ namespace.put("feature", Collections.singletonList("foo.bar=6"));
+ namespace.put("dry_run", false);
+ String upgradeOutput = ToolsTestUtils.captureStandardOut(() ->
assertTrue(assertThrows(TerseException.class, () ->
FeatureCommand.handleUpgrade(new Namespace(namespace), buildAdminClient()))
+ .getMessage().contains("1 out of 2 operation(s) failed.")));
+ assertEquals(format("foo.bar was upgraded to 6.%n" +
+ "Could not upgrade metadata.version to 5. Can't upgrade to
lower version."), upgradeOutput);
+ }
+
+ @Test
+ public void testHandleUpgradeDryRun() {
+ Map<String, Object> namespace = new HashMap<>();
+ namespace.put("metadata", "3.3-IV1");
+ namespace.put("feature", Collections.singletonList("foo.bar=6"));
+ namespace.put("dry_run", true);
+ String upgradeOutput = ToolsTestUtils.captureStandardOut(() ->
assertTrue(assertThrows(TerseException.class, () ->
FeatureCommand.handleUpgrade(new Namespace(namespace), buildAdminClient()))
+ .getMessage().contains("1 out of 2 operation(s) failed.")));
+ assertEquals(format("foo.bar can be upgraded to 6.%n" +
+ "Can not upgrade metadata.version to 5. Can't upgrade to lower
version."), upgradeOutput);
+ }
+
+ @Test
+ public void testHandleDowngrade() {
+ Map<String, Object> namespace = new HashMap<>();
+ namespace.put("metadata", "3.3-IV3");
+ namespace.put("feature", Collections.singletonList("foo.bar=1"));
+ namespace.put("dry_run", false);
+ String upgradeOutput = ToolsTestUtils.captureStandardOut(() ->
assertTrue(assertThrows(TerseException.class, () ->
FeatureCommand.handleDowngrade(new Namespace(namespace), buildAdminClient()))
+ .getMessage().contains("1 out of 2 operation(s) failed.")));
+ assertEquals(format("foo.bar was downgraded to 1.%n" +
+ "Could not downgrade metadata.version to 7. Can't downgrade to
newer version."), upgradeOutput);
+ }
+
+ @Test
+ public void testHandleDowngradeDryRun() {
+ Map<String, Object> namespace = new HashMap<>();
+ namespace.put("metadata", "3.3-IV3");
+ namespace.put("feature", Collections.singletonList("foo.bar=1"));
+ namespace.put("dry_run", true);
+ String upgradeOutput = ToolsTestUtils.captureStandardOut(() ->
assertTrue(assertThrows(TerseException.class, () ->
FeatureCommand.handleDowngrade(new Namespace(namespace), buildAdminClient()))
+ .getMessage().contains("1 out of 2 operation(s) failed.")));
+ assertEquals(format("foo.bar can be downgraded to 1.%n" +
+ "Can not downgrade metadata.version to 7. Can't downgrade to
newer version."), upgradeOutput);
+ }
+
+ @Test
+ public void testHandleDisable() {
+ Map<String, Object> namespace = new HashMap<>();
+ namespace.put("metadata", "3.3-IV3");
+ namespace.put("feature", Arrays.asList("foo.bar", "metadata.version",
"quux"));
+ namespace.put("dry_run", false);
+ String disableOutput = ToolsTestUtils.captureStandardOut(() ->
assertTrue(assertThrows(TerseException.class, () ->
FeatureCommand.handleDisable(new Namespace(namespace), buildAdminClient()))
+ .getMessage().contains("1 out of 3 operation(s) failed.")));
+ assertEquals(format("quux was disabled.%n" +
+ "foo.bar was disabled.%n" +
+ "Could not disable metadata.version. Can't downgrade below
4"), disableOutput);
+ }
+
+ @Test
+ public void testHandleDisableDryRun() {
+ Map<String, Object> namespace = new HashMap<>();
+ namespace.put("metadata", "3.3-IV3");
+ namespace.put("feature", Arrays.asList("foo.bar", "metadata.version",
"quux"));
+ namespace.put("dry_run", true);
+ String disableOutput = ToolsTestUtils.captureStandardOut(() ->
assertTrue(assertThrows(TerseException.class, () ->
FeatureCommand.handleDisable(new Namespace(namespace), buildAdminClient()))
Review Comment:
It's hard to read this assertion. Could we split them out? ex:
```java
Throwable t = assertThrows(TerseException.class, () ->
FeatureCommand.handleDisable(new Namespace(namespace), buildAdminClient());
assertTrue(t.getMessage(), ...)
...
```
WDYT?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
