AndrewJSchofield commented on code in PR #19808:
URL: https://github.com/apache/kafka/pull/19808#discussion_r2112143179
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -342,6 +342,42 @@ object ConfigCommand extends Logging {
}
private def describeResourceConfig(adminClient: Admin, entityType: String,
entityName: Option[String], describeAll: Boolean): Unit = {
+ if (!describeAll) {
+ entityName.foreach { name =>
+ entityType match {
+ case TopicType =>
+ Topic.validate(name)
+ if (!adminClient.listTopics(new
ListTopicsOptions().listInternal(true)).names.get.contains(name)) {
+ System.out.println(s"The $entityType '$name' doesn't exist and
doesn't have dynamic config.")
Review Comment:
Alternatively, and this is what they do elsewhere in this tool, use
`${entityType.dropRight(1)}`.
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -342,6 +342,42 @@ object ConfigCommand extends Logging {
}
private def describeResourceConfig(adminClient: Admin, entityType: String,
entityName: Option[String], describeAll: Boolean): Unit = {
+ if (!describeAll) {
+ entityName.foreach { name =>
+ entityType match {
+ case TopicType =>
+ Topic.validate(name)
+ if (!adminClient.listTopics(new
ListTopicsOptions().listInternal(true)).names.get.contains(name)) {
+ System.out.println(s"The $entityType '$name' doesn't exist and
doesn't have dynamic config.")
Review Comment:
The variable `entityType` contains the pluralized noun such as topics or
groups. As a result, the message is not grammatical, for example `The groups
'G2' doesn't exist and doesn't have dynamic config.`. This problem is evident
for all entity types.
Probably easiest here is just to use a slightly different string for each
type such as `s"The topic '$name' doesn't exist and doesn't have dynamic
config."`.
##########
tools/src/main/java/org/apache/kafka/tools/ClientMetricsCommand.java:
##########
@@ -156,6 +156,12 @@ public void
describeClientMetrics(ClientMetricsCommandOptions opts) throws Excep
List<String> entities;
if (entityNameOpt.isPresent()) {
+ if
(adminClient.listConfigResources(Set.of(ConfigResource.Type.CLIENT_METRICS),
new ListConfigResourcesOptions())
+ .all().get(30, TimeUnit.SECONDS).stream()
+ .noneMatch(resource ->
resource.name().equals(entityNameOpt.get()))) {
+ System.out.println("The client-metric " +
entityNameOpt.get() + " doesn't exist and doesn't have dynamic config.");
Review Comment:
I think I'd use `client metrics resource`. This is the terminology in the
usage message for this tool. It's a bit of a mouthful, but `client-metric`
sounds like a metric from a client, not something uses to configure them.
--
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]
