AndrewJSchofield commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1371529124
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -445,6 +446,21 @@ object ConfigCommand extends Logging {
if (unknownConfigs.nonEmpty)
throw new IllegalArgumentException(s"Only connection quota configs
can be added for '${ConfigType.Ip}' using --bootstrap-server. Unexpected config
names: ${unknownConfigs.mkString(",")}")
alterQuotaConfigs(adminClient, entityTypes, entityNames,
configsToBeAddedMap, configsToBeDeleted)
+ case ConfigType.ClientMetrics =>
+ val oldConfig = getResourceConfig(adminClient, entityTypeHead,
entityNameHead, includeSynonyms = false, describeAll = false)
+ .map { entry => (entry.name, entry) }.toMap
+
+ // fail the command if any of the configs to be deleted does not exist
+ val invalidConfigs = configsToBeDeleted.filterNot(oldConfig.contains)
+ if (invalidConfigs.nonEmpty)
+ throw new InvalidConfigurationException(s"Invalid config(s):
${invalidConfigs.mkString(",")}")
Review Comment:
This error message could be clearer.
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -536,6 +552,8 @@ object ConfigCommand extends Logging {
adminClient.listTopics(new
ListTopicsOptions().listInternal(true)).names().get().asScala.toSeq
case ConfigType.Broker | BrokerLoggerConfigType =>
adminClient.describeCluster(new
DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq :+
BrokerDefaultEntityName
+ case ConfigType.ClientMetrics =>
+ adminClient.describeCluster(new
DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq
Review Comment:
I expect this needs to be `adminClient.describeConfigs`.
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -576,6 +594,8 @@ object ConfigCommand extends Logging {
if (entityName.nonEmpty)
validateBrokerId()
(ConfigResource.Type.BROKER_LOGGER, None)
+ case ConfigType.ClientMetrics =>
+ (ConfigResource.Type.CLIENT_METRICS,
Some(ConfigEntry.ConfigSource.DYNAMIC_CLIENT_METRICS_CONFIG))
Review Comment:
Probably ought to have a non-empty entity name, or describeAll.
--
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]
