Skip to content

KAFKA-19749 The version-mapping of kafka-features.sh should work without requiring the bootstrap #20619

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
+34 −11
Open

KAFKA-19749 The version-mapping of kafka-features.sh should work without requiring the bootstrap #20619

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
63dac7d
Add a test to catch exception when calling version-mapping without a …
jack2012aa Sep 30, 2025
3940027
Add a test to catch exception when calling a remote command without a…
jack2012aa Sep 30, 2025
c8ccf23
Set bootstrapGroup not-required and check their existence if they are…
jack2012aa Sep 30, 2025
d6679a8
Use error message from initializeBootstrapProperties
jack2012aa Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions tools/src/main/java/org/apache/kafka/tools/FeatureCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ static void execute(String... args) throws Exception {
.newArgumentParser("kafka-features")
.defaultHelp(true)
.description("This tool manages feature flags in Kafka.");
MutuallyExclusiveGroup bootstrapGroup = parser.addMutuallyExclusiveGroup().required(true);
MutuallyExclusiveGroup bootstrapGroup = parser.addMutuallyExclusiveGroup();
bootstrapGroup.addArgument("--bootstrap-server")
.help("A comma-separated list of host:port pairs to use for establishing the connection to the Kafka cluster.");
bootstrapGroup.addArgument("--bootstrap-controller")
Expand All @@ -92,14 +92,27 @@ static void execute(String... args) throws Exception {
addVersionMappingParser(subparsers);
addFeatureDependenciesParser(subparsers);

Namespace namespace = parser.parseArgsOrFail(args);
Namespace namespace = parser.parseArgs(args);
String command = namespace.getString("command");
if (command.equals("version-mapping")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach allows version-mapping to be used with both bootstrap-server and bootstrap-controller, which seems a bit odd to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version-mapping and feature-dependencies subcommands don't use bootstrap-server and bootstrap-controller arguments at all. The best practice is to let these arguments be required arguments of other subcommands. But I am not sure whether this change will require an KIP or worth one. Do you have any suggestion?

Or, we can check whether user uses these arguments with version-mapping and feature-dependencies, which makes the code more ugly but can provide user more information.

handleVersionMapping(namespace, Feature.PRODUCTION_FEATURES);
return;
} else if (command.equals("feature-dependencies")) {
handleFeatureDependencies(namespace, Feature.PRODUCTION_FEATURES);
return;
}
String configPath = namespace.getString("command_config");
Properties properties = (configPath == null) ? new Properties() : Utils.loadProps(configPath);

CommandLineUtils.initializeBootstrapProperties(properties,
Optional.ofNullable(namespace.getString("bootstrap_server")),
Optional.ofNullable(namespace.getString("bootstrap_controller")));
try {
CommandLineUtils.initializeBootstrapProperties(properties,
Optional.ofNullable(namespace.getString("bootstrap_server")),
Optional.ofNullable(namespace.getString("bootstrap_controller")));
} catch (Exception e) {
// bootstrap_server and bootstrap_controller are in a mutually exclusive group,
// so the exception happens only when both of them are missing
throw new ArgumentParserException("one of the arguments --bootstrap-server --bootstrap-controller is required", parser);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about new ArgumentParserException(e.getMessage(), parser)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! Yes, it's better to use the existing message rather than write a new one. The code has been updated.

}

try (Admin adminClient = Admin.create(properties)) {
switch (command) {
Expand All @@ -115,12 +128,6 @@ static void execute(String... args) throws Exception {
case "disable":
handleDisable(namespace, adminClient);
break;
case "version-mapping":
handleVersionMapping(namespace, Feature.PRODUCTION_FEATURES);
break;
case "feature-dependencies":
handleFeatureDependencies(namespace, Feature.PRODUCTION_FEATURES);
break;
default:
throw new TerseException("Unknown command " + command);
}
Expand Down
15 changes: 15 additions & 0 deletions tools/src/test/java/org/apache/kafka/tools/FeatureCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,21 @@ public void testHandleVersionMappingWithInvalidReleaseVersion() {
MetadataVersion.MINIMUM_VERSION, MetadataVersion.latestTesting()), exception2.getMessage());
}

@Test
public void testHandleVersionMappingWithoutBootstrap() {
Map.Entry<String, String> output = ToolsTestUtils.grabConsoleOutputAndError(() -> FeatureCommand.mainNoExit("version-mapping"));
assertEquals("", output.getValue());
MetadataVersion metadataVersion = MetadataVersion.latestProduction();
assertTrue(output.getKey().contains("metadata.version=" + metadataVersion.featureLevel() + " (" + metadataVersion.version() + ")"),
"Output did not contain expected Metadata Version: " + output.getKey());
}

@Test
public void testHandleRemoteCommandWithoutBootstrap() {
String errorMessage = ToolsTestUtils.grabConsoleError(() -> FeatureCommand.mainNoExit("upgrade"));
assertTrue(errorMessage.contains("one of the arguments --bootstrap-server --bootstrap-controller is required"));
}

@Test
public void testHandleFeatureDependenciesForFeatureWithDependencies() {
Map<String, Object> namespace = new HashMap<>();
Expand Down
Loading