dajac commented on code in PR #16887:
URL: https://github.com/apache/kafka/pull/16887#discussion_r1729122211
##########
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##########
@@ -256,6 +278,32 @@ public void testDynamicBrokerConfigUpdateUsingKraft()
throws Exception {
}
}
+ @ClusterTest(types = {Type.KRAFT, Type.CO_KRAFT}, serverProperties = {
+ @ClusterConfigProperty(key = "group.coordinator.new.enable", value =
"true"),
+ @ClusterConfigProperty(key = "group.coordinator.rebalance.protocols",
value = "classic,consumer"),
Review Comment:
Can we use the constants instead of the raw strings? It should work from
here.
##########
clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java:
##########
@@ -243,6 +245,8 @@ private MockAdminClient(
this.brokerLogDirs = brokerLogDirs;
this.brokerConfigs = new ArrayList<>();
this.clientMetricsConfigs = new HashMap<>();
+ this.groupConfigs = new HashMap<>();
+ this.defaultGroupConfigs = generateDefaultGroupConfigs();
Review Comment:
Should we rather define a constant for the default group configs? Thinking a
bit more about those too. I wonder if we should rather extend the builder to
let the user passes the default group config that he wants to use instead of
having some defaults defined here. Then, we can have a constant on the user
side which is a litter clearer in my opinion because you see them when you read
the tests. What do you think?
##########
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##########
@@ -126,6 +137,17 @@ public void testNullStatusOnKraftCommandAlterUserQuota() {
assertTrue(StringUtils.isBlank(message), message);
}
+ @ClusterTest(types = {Type.CO_KRAFT, Type.KRAFT})
+ public void testNullStatusOnKraftCommandAlterGroup() {
+ Stream<String> command = Stream.concat(quorumArgs(), Stream.of(
+ "--entity-type", "groups",
+ "--entity-name", "group",
Review Comment:
Should we also test with the `--group` alias?
##########
core/src/test/java/kafka/admin/ConfigCommandTest.java:
##########
@@ -1918,6 +1943,148 @@ public void
shouldNotSupportAlterClientMetricsWithZookeeper() {
assertEquals("client-metrics is not a known entityType. Should be one
of List(topics, clients, users, brokers, ips)", exception.getMessage());
}
+ @Test
+ public void shouldAlterGroupConfig() {
+ Node node = new Node(1, "localhost", 9092);
+ verifyAlterGroupConfig(node, "group", Arrays.asList("--entity-name",
"group"));
+ }
+
+ private void verifyAlterGroupConfig(Node node, String resourceName,
List<String> resourceOpts) {
+ List<String> optsList = concat(Arrays.asList("--bootstrap-server",
"localhost:9092",
+ "--entity-type", "groups",
+ "--alter",
+ "--delete-config", "consumer.session.timeout.ms",
+ "--add-config", "consumer.heartbeat.interval.ms=6000"),
resourceOpts);
+ ConfigCommand.ConfigCommandOptions alterOpts = new
ConfigCommand.ConfigCommandOptions(toArray(optsList));
+
+ ConfigResource resource = new
ConfigResource(ConfigResource.Type.GROUP, resourceName);
+ List<ConfigEntry> configEntries = Collections.singletonList(new
ConfigEntry("consumer.session.timeout.ms", "45000",
+ ConfigEntry.ConfigSource.DYNAMIC_GROUP_CONFIG, false, false,
Collections.emptyList(),
+ ConfigEntry.ConfigType.UNKNOWN, null));
+ KafkaFutureImpl<Map<ConfigResource, Config>> future = new
KafkaFutureImpl<>();
+ future.complete(Collections.singletonMap(resource, new
Config(configEntries)));
+ DescribeConfigsResult describeResult =
mock(DescribeConfigsResult.class);
+ when(describeResult.all()).thenReturn(future);
+
+ KafkaFutureImpl<Void> alterFuture = new KafkaFutureImpl<>();
+ alterFuture.complete(null);
+ AlterConfigsResult alterResult = mock(AlterConfigsResult.class);
+ when(alterResult.all()).thenReturn(alterFuture);
+
+ MockAdminClient mockAdminClient = new
MockAdminClient(Collections.singletonList(node), node) {
+ @Override
+ public synchronized DescribeConfigsResult
describeConfigs(Collection<ConfigResource> resources, DescribeConfigsOptions
options) {
+ assertFalse(options.includeSynonyms(), "Config synonyms
requested unnecessarily");
+ assertEquals(1, resources.size());
+ ConfigResource res = resources.iterator().next();
+ assertEquals(ConfigResource.Type.GROUP, res.type());
+ assertEquals(resourceName, res.name());
+ return describeResult;
+ }
+
+ @Override
+ public synchronized AlterConfigsResult
incrementalAlterConfigs(Map<ConfigResource, Collection<AlterConfigOp>> configs,
AlterConfigsOptions options) {
+ assertEquals(1, configs.size());
+ Map.Entry<ConfigResource, Collection<AlterConfigOp>> entry =
configs.entrySet().iterator().next();
+ ConfigResource res = entry.getKey();
+ Collection<AlterConfigOp> alterConfigOps = entry.getValue();
+ assertEquals(ConfigResource.Type.GROUP, res.type());
+ assertEquals(2, alterConfigOps.size());
+
+ List<AlterConfigOp> expectedConfigOps = Arrays.asList(
+ new AlterConfigOp(new
ConfigEntry("consumer.heartbeat.interval.ms", "6000"),
AlterConfigOp.OpType.SET),
+ new AlterConfigOp(new
ConfigEntry("consumer.session.timeout.ms", ""), AlterConfigOp.OpType.DELETE)
+ );
+ assertEquals(expectedConfigOps.size(), alterConfigOps.size());
+ Iterator<AlterConfigOp> alterConfigOpsIter =
alterConfigOps.iterator();
+ for (AlterConfigOp expectedConfigOp : expectedConfigOps) {
+ assertEquals(expectedConfigOp, alterConfigOpsIter.next());
+ }
+ return alterResult;
+ }
+ };
+ ConfigCommand.alterConfig(mockAdminClient, alterOpts);
+ verify(describeResult).all();
+ verify(alterResult).all();
+ }
+
+ @Test
+ public void shouldDescribeGroupConfigWithoutEntityName() {
+ ConfigCommand.ConfigCommandOptions describeOpts = new
ConfigCommand.ConfigCommandOptions(toArray("--bootstrap-server",
"localhost:9092",
+ "--entity-type", "groups",
+ "--describe"));
+
+ ConfigResource resourceCustom = new
ConfigResource(ConfigResource.Type.GROUP, "group");
+ ConfigEntry configEntry = new
ConfigEntry("consumer.heartbeat.interval.ms", "6000");
+ KafkaFutureImpl<Map<ConfigResource, Config>> future = new
KafkaFutureImpl<>();
+ DescribeConfigsResult describeResult =
mock(DescribeConfigsResult.class);
+ when(describeResult.all()).thenReturn(future);
+
+ Node node = new Node(1, "localhost", 9092);
+ MockAdminClient mockAdminClient = new
MockAdminClient(Collections.singletonList(node), node) {
+ @Override
+ public synchronized DescribeConfigsResult
describeConfigs(Collection<ConfigResource> resources, DescribeConfigsOptions
options) {
+ assertTrue(options.includeSynonyms());
+ assertEquals(1, resources.size());
+ ConfigResource resource = resources.iterator().next();
+ assertEquals(ConfigResource.Type.GROUP, resource.type());
+ assertEquals(resourceCustom.name(), resource.name());
+ future.complete(Collections.singletonMap(resourceCustom, new
Config(Collections.singletonList(configEntry))));
+ return describeResult;
+ }
+ };
+
mockAdminClient.incrementalAlterConfigs(Collections.singletonMap(resourceCustom,
+ Collections.singletonList(new AlterConfigOp(configEntry,
AlterConfigOp.OpType.SET))), new AlterConfigsOptions());
+ ConfigCommand.describeConfig(mockAdminClient, describeOpts);
+ verify(describeResult).all();
+ }
+
+ @Test
+ public void shouldNotAlterGroupConfigWithoutEntityName() {
+ ConfigCommand.ConfigCommandOptions alterOpts = new
ConfigCommand.ConfigCommandOptions(toArray("--bootstrap-server",
"localhost:9092",
+ "--entity-type", "groups",
+ "--alter",
+ "--add-config", "consumer.heartbeat.interval.ms=6000"));
+
+ IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class, alterOpts::checkArgs);
+ assertEquals("an entity name must be specified with --alter of
groups", exception.getMessage());
+ }
+
+ @Test
+ public void shouldNotSupportAlterGroupConfigWithZookeeperArg() {
+ ConfigCommand.ConfigCommandOptions alterOpts = new
ConfigCommand.ConfigCommandOptions(toArray("--zookeeper", ZK_CONNECT,
+ "--entity-name", "group",
+ "--entity-type", "groups",
+ "--alter",
+ "--add-config", "consumer.heartbeat.interval.ms=6000"));
+
+ IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class, alterOpts::checkArgs);
+ assertEquals("Invalid entity type groups, the entity type must be one
of users, brokers with a --zookeeper argument", exception.getMessage());
+ }
+
+ @Test
+ public void shouldNotSupportDescribeGroupConfigWithZookeeperArg() {
+ ConfigCommand.ConfigCommandOptions describeOpts = new
ConfigCommand.ConfigCommandOptions(toArray("--zookeeper", ZK_CONNECT,
+ "--entity-name", "group",
+ "--entity-type", "groups",
+ "--describe"));
+
+ IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class, describeOpts::checkArgs);
+ assertEquals("Invalid entity type groups, the entity type must be one
of users, brokers with a --zookeeper argument", exception.getMessage());
+ }
+
+ @Test
+ public void shouldNotSupportAlterGroupConfigWithZookeeper() {
+ ConfigCommand.ConfigCommandOptions alterOpts = new
ConfigCommand.ConfigCommandOptions(toArray("--zookeeper", ZK_CONNECT,
+ "--entity-name", "group",
+ "--entity-type", "groups",
+ "--alter",
+ "--add-config", "consumer.heartbeat.interval.ms=6000"));
+
+ IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class, () ->
ConfigCommand.alterConfigWithZk(null, alterOpts, DUMMY_ADMIN_ZK_CLIENT));
+ assertEquals("groups is not a known entityType. Should be one of
List(topics, clients, users, brokers, ips)", exception.getMessage());
+ }
+
Review Comment:
Should we also add a unit test for the `--group` alias?
--
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]
