KAFKA-14588: Move ConfigCommand to tools module by mimaison · Pull Request #22013 · apache/kafka

mimaison · 2026-04-09T16:21:01Z

Rewrite ConfigCommand in Java and move it to the tools module

Reviewers: Ken Huang s7133700@gmail.com, Christo Lolov
lolovc@amazon.com, Chia-Ping Tsai chia7712@gmail.com, Nick Guo
lansg0504@gmail.com

mimaison · 2026-04-09T16:21:25Z

    }

-    @ClusterTest(types = {Type.CO_KRAFT, Type.KRAFT})
-    public void testIntervalMsParser(ClusterInstance clusterInstance) {


This was moved to ConfigCommandIntegrationTest

mimaison · 2026-04-09T16:22:13Z


    val adminClient = adminClients.head
-    alterSslKeystoreUsingConfigCommand(sslProperties1, SecureExternal)
+    alterSslKeystore(sslProperties1, SecureExternal)


I did not see the point in using the tool instead of the Admin API

Rewrite ConfigCommand in Java and move it to the tools module

m1a2st

Thanks for the patch, left some comments

m1a2st · 2026-04-10T14:53:24Z

+                    boolean noMatchInResources = listGroupConfigResources(adminClient)
+                            .map(resources -> resources.stream().noneMatch(resource -> resource.name().equals(name)))
+                            .orElse(true);
+                    if (noMatchInGroups && noMatchInResources) {


The semantics here don’t match the Scala version. .orElse(true) returns true when listGroupConfigResources is empty, whereas Scala’s .exists(...) returns false for an empty Option.

Good catch! I agree this should be orElse(false)

m1a2st · 2026-04-10T14:56:37Z

+ * Alternatively, --user-defaults, --client-defaults, --broker-defaults, or --ip-defaults may be specified in place of
+ * --entity-type <users|clients|brokers|ips> --entity-default, respectively.
+ */
+class ConfigCommand  {


Should it be public class?

m1a2st · 2026-04-10T15:01:35Z

+        }
+    }
+
+


mimaison · 2026-04-10T20:27:58Z

Thanks @m1a2st for the review! I pushed an update

clolov

Looks great to me 😊! I will let you decide whether to merge it or wait for one more review from @m1a2st

m1a2st

Thanks for the update!

m1a2st · 2026-04-15T14:15:06Z

+                String name = entityEntries.get(entityType);
+                if (name == null) {
+                    return Optional.empty();
+                }
+                String typeStr = switch (entityType) {
+                    case ClientQuotaEntity.USER -> "user-principal";
+                    case ClientQuotaEntity.CLIENT_ID -> "client-id";
+                    case ClientQuotaEntity.IP -> "ip";
+                    default -> throw new IllegalArgumentException("Unknown entity type: " + entityType);
+                };
+                String result = name.isEmpty() ? "the default " + typeStr : typeStr + " '" + name + "'";
+                return Optional.of(result);


I think this logic differs from the Scala implementation. In Scala, map.get(entityType) returns an Option[String]:

Key exists with a null value → Some(null) (represents the default entity)

Key does not exist → None

In the Java version, entityEntries.get(entityType), which returns null for both cases, when the key is absent and when the key is present with a null value.

As a result, default entities are incorrectly treated as missing, which can lead to incorrect behavior.

It's unclear if the value can ever be null. That said, it's easy to handle that case so I pushed an update. Thanks

m1a2st · 2026-04-15T14:15:59Z

+ * Alternatively, --user-defaults, --client-defaults, --broker-defaults, or --ip-defaults may be specified in place of
+ * --entity-type <users|clients|brokers|ips> --entity-default, respectively.
+ */
+public class ConfigCommand  {


There are two spaces.

Suggested change

public class ConfigCommand {

public class ConfigCommand {

m1a2st · 2026-04-15T14:24:00Z

+            // (KIP-1142) 4.1+ admin client vs older broker: treat UnsupportedVersionException and ClusterAuthorizationException as None
+            if (ee.getCause() instanceof UnsupportedVersionException) return Optional.empty();
+            if (ee.getCause() instanceof ClusterAuthorizationException) return Optional.empty();
+            else throw (Exception)  ee.getCause();


Suggested change

else throw (Exception) ee.getCause();

else throw (Exception) ee.getCause();

chia7712 · 2026-04-16T16:41:12Z

+                        .filter(options::has)
+                        .map(options::valueOf)
+                        .forEach(ipEntity -> {
+                            if (!isValidIpEntity(ipEntity))


We could streamline this lambda if isValidIpEntity threw an exception directly.

static void validateIpEntity(String ipEntity, String entityType) { try { InetAddress.getByName(ipEntity); } catch (UnknownHostException uhe) { throw new IllegalArgumentException("The entity name for " + entityType + " must be a valid IP or resolvable host, but it is: " + ipEntity); } }

if (hasEntityName && entityTypeVals.contains(IP_TYPE)) { Stream.of(entityName, ip) .filter(options::has) .map(options::valueOf) .forEach(ipEntity -> validateIpEntity(ipEntity, entityTypeVals.get(0))); }

Yes that's slightly more elegant, good idea!

m1a2st

Thanks, LGTM

chia7712

LGTM. The comments I left can be addressed in a follow-up. We'll open a separate PR for them so we can merge this big migration first.

chia7712 · 2026-04-18T02:26:43Z

+    private static final Logger LOG = LoggerFactory.getLogger(ConfigCommand.class);
+
+    private static final String BROKER_DEFAULT_ENTITY_NAME = "";
+    private static final List<String> BROKER_SUPPORTED_CONFIG_TYPES;


private static final List<String> BROKER_SUPPORTED_CONFIG_TYPES = Stream.concat( Stream.of(BROKER_LOGGER_CONFIG_TYPE), Arrays.stream(ConfigType.values()).map(ConfigType::value)).toList();

chia7712 · 2026-04-18T02:27:55Z

+            }
+        }
+
+        List<String> entities;


we could use Set<String> to avoid unnecessary copy

chia7712 · 2026-04-18T02:29:04Z

+            }
+        }
+
+        for (String entity : entities) {


It seems we could use batch query here for a slight optimization

chia7712 · 2026-04-18T02:29:48Z

+            throw new InvalidConfigurationException("Invalid config(s): " + String.join(",", invalidConfigs));
+
+        ConfigResource configResource = new ConfigResource(resourceType, entityNameHead);
+        AlterConfigsOptions alterOptions = new AlterConfigsOptions().timeoutMs(30000).validateOnly(false);


.validateOnly(false); is redundant

chia7712 · 2026-04-18T02:29:59Z

+                throw new InvalidConfigurationException("Invalid broker logger(s): " + String.join(",", invalidBrokerLoggers));
+
+            ConfigResource configResource = new ConfigResource(ConfigResource.Type.BROKER_LOGGER, entityName);
+            AlterConfigsOptions alterOptions = new AlterConfigsOptions().timeoutMs(30000).validateOnly(false);


chia7712 · 2026-04-18T02:30:04Z

+        }
+        ClientQuotaEntity entity = new ClientQuotaEntity(alterEntityMap);
+
+        AlterClientQuotasOptions alterOptions = new AlterClientQuotasOptions().validateOnly(false);


Rancho-7 · 2026-04-18T12:36:41Z

Hi @chia7712, I would like to handle this.

chia7712 · 2026-04-18T12:37:27Z

I would like to handle this.

go ahead

Rancho-7 · 2026-04-18T12:39:29Z

+ * An entity described or altered by the command may be one of:
+ * <ul>
+ *     <li> topic: --topic <topic> OR --entity-type topics --entity-name <topic>
+ *     <li> client: --client <client> OR --entity-type clients --entity-name <client-id>


@mimaison btw, should we update <client> to <client-id> here? It seems <client> was preserved from the Scala source, but <client-id> would be more internally consistent. If that sounds good to you, I'd be happy to include it in the follow-up PR.

+1 to use <client-id>

mimaison commented Apr 9, 2026

View reviewed changes

github-actions Bot added core Kafka Broker tools clients labels Apr 9, 2026

mimaison commented Apr 9, 2026

View reviewed changes

KAFKA-14588: Move ConfigCommand to tools module

58ec37d

Rewrite ConfigCommand in Java and move it to the tools module

mimaison force-pushed the kafka-14588 branch from 664180c to 58ec37d Compare April 9, 2026 16:50

m1a2st reviewed Apr 10, 2026

View reviewed changes

Address feedback from Ken

7f7a9a2

clolov approved these changes Apr 15, 2026

View reviewed changes

m1a2st reviewed Apr 15, 2026

View reviewed changes

Address 2nd feedback from Ken

7883479

chia7712 reviewed Apr 16, 2026

View reviewed changes

Refactor validateIpEntity

43bf364

m1a2st approved these changes Apr 18, 2026

View reviewed changes

chia7712 approved these changes Apr 18, 2026

View reviewed changes

chia7712 merged commit 46773ce into apache:trunk Apr 18, 2026
35 of 37 checks passed

Rancho-7 reviewed Apr 18, 2026

View reviewed changes

Rancho-7 mentioned this pull request Apr 22, 2026

MINOR: Optimize describeConfigs queries and address cleanups for ConfigCommand #22114

Open

	else throw (Exception) ee.getCause();
	else throw (Exception) ee.getCause();

Conversation

mimaison commented Apr 9, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m1a2st 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!

mimaison commented Apr 10, 2026

Uh oh!

clolov left a comment

Choose a reason for hiding this comment

Uh oh!

m1a2st left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m1a2st Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

m1a2st left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 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!

Uh oh!

Rancho-7 commented Apr 18, 2026

Uh oh!

chia7712 commented Apr 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mimaison commented Apr 9, 2026 •

edited by github-actions Bot

Loading

m1a2st left a comment •

edited

Loading

m1a2st Apr 15, 2026 •

edited

Loading