chia7712 commented on code in PR #16887:
URL: https://github.com/apache/kafka/pull/16887#discussion_r1718910571
##########
core/src/test/java/kafka/admin/ConfigCommandTest.java:
##########
@@ -1918,6 +1918,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() {
Review Comment:
Do you plan to add IT for support of group? or it is a future work?
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -562,6 +543,8 @@ object ConfigCommand extends Logging {
adminClient.describeCluster(new
DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq :+
BrokerDefaultEntityName
case ConfigType.CLIENT_METRICS =>
adminClient.listClientMetricsResources().all().get().asScala.map(_.name).toSeq
+ case ConfigType.GROUP =>
+ adminClient.listConsumerGroups(new
ListConsumerGroupsOptions().withTypes(Seq(GroupType.CONSUMER).toSet.asJava)).all.get.asScala.map(_.groupId).toSeq
Review Comment:
> So, I can set configs for classic consumer groups, but --describe without
a group name will not show them.
does `Share` have similar issue?
> Once that KIP is approved, I'll improve this area.
our of curiosity. I feel your suggestion `Seq(GroupType.CONSUMER,
GroupType.CLASSIC)` is good enough if we only care about the configs of
`CONSUMER` and `CLASSIC` . what is the improvement of this case introduced by
KIP-1043?
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -580,6 +563,23 @@ object ConfigCommand extends Logging {
}
}
+ private def alterResourceConfig(adminClient: Admin, entityTypeHead: String,
entityNameHead: String, configsToBeDeleted: Seq[String], configsToBeAdded:
Map[String, ConfigEntry], resourceType: ConfigResource.Type): Unit = {
+ val oldConfig = getResourceConfig(adminClient, entityTypeHead,
entityNameHead, includeSynonyms = false, describeAll = false)
Review Comment:
In reviewing it, I notice that `admin.describeConfigs` will return static
configs even if the group is nonexistent. IMHO, that should return
GROUP_ID_NOT_FOUND. @AndrewJSchofield @DL1231 WDYT? I can file a jira to fix it
if it is NOT expected behavior.
--
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]
