AndrewJSchofield commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1390269923
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -791,10 +811,10 @@ object ConfigCommand extends Logging {
val describeOpt = parser.accepts("describe", "List configs for the given
entity.")
val allOpt = parser.accepts("all", "List all configs for the given topic,
broker, or broker-logger entity (includes static configuration when the entity
type is brokers)")
- val entityType = parser.accepts("entity-type", "Type of entity
(topics/clients/users/brokers/broker-loggers/ips)")
+ val entityType = parser.accepts("entity-type", "Type of entity
(topics/clients/users/brokers/broker-loggers/ips/client-metrics)")
.withRequiredArg
.ofType(classOf[String])
- val entityName = parser.accepts("entity-name", "Name of entity (topic
name/client id/user principal name/broker id/ip)")
+ val entityName = parser.accepts("entity-name", "Name of entity (topic
name/client id/user principal name/broker id/ip/client metrics subscription
name)")
Review Comment:
Strictly speaking, this is not the client metrics subscription name. It's
the name of the client metrics config resource. That's quite a mouthful, so I'd
put `/ip/client metrics`.
##########
clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java:
##########
@@ -916,6 +925,34 @@ synchronized private Throwable
handleIncrementalResourceAlteration(
topicMetadata.configs = newMap;
return null;
}
+ case CLIENT_METRICS: {
+ String subscriptionName = resource.name();
Review Comment:
Again, `resourceName` I think is better.
##########
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 =>
+ throw new InvalidRequestException("Client metrics entity-name is
required")
Review Comment:
I've written `kafka-client-metrics.sh` also which will be in a new PR once
this one is merged.
##########
clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java:
##########
@@ -823,6 +825,13 @@ synchronized private Config
getResourceDescription(ConfigResource resource) {
}
throw new UnknownTopicOrPartitionException("Resource " +
resource + " not found.");
}
+ case CLIENT_METRICS: {
+ String subscriptionName = resource.name();
Review Comment:
I'd call it `resourceName`. It's not the subscription name - the
subscription is the set of metrics which is sent by the broker to a specific
client instance, and that's composed of the client metrics config resources
which match the client.
##########
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 =>
+ throw new InvalidRequestException("Client metrics entity-name is
required")
Review Comment:
Precisely.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
