KAFKA-18904: kafka-configs.sh return resource doesn't exist message [3/N]#19808
KAFKA-18904: kafka-configs.sh return resource doesn't exist message [3/N]#19808AndrewJSchofield merged 6 commits intoapache:trunkfrom
Conversation
…urce doesn't exist message Signed-off-by: PoAn Yang <payang@apache.org>
DL1231
left a comment
DL1231
left a comment
There was a problem hiding this comment.
@FrankYang0529 Thanks for the PR, overall LGTM.
| .map(name -> new ConfigResource(entry.getKey(), name))) | ||
| .collect(Collectors.toList()); | ||
| return new ListConfigResourcesResult(KafkaFuture.completedFuture(resources)); | ||
|
|
| return | ||
| } | ||
| case ClientMetricsType => | ||
| if (adminClient.listConfigResources(java.util.Set.of(ConfigResource.Type.CLIENT_METRICS), new ListConfigResourcesOptions).all.get |
There was a problem hiding this comment.
nit: Can't this just be util.Set.of?
There was a problem hiding this comment.
I prefer to leave this as java.util.xxx because we already used this pattern like:
We can do some refactor for Scala code in core module after this PR. Or we can refactor it when migrating to Java eventually.
| } | ||
| case GroupType => | ||
| if (adminClient.listGroups().all.get.stream.noneMatch(_.groupId() == name) && | ||
| adminClient.listConfigResources(java.util.Set.of(ConfigResource.Type.GROUP), new ListConfigResourcesOptions).all.get |
Signed-off-by: PoAn Yang <payang@apache.org>
AndrewJSchofield
left a comment
AndrewJSchofield
left a comment
There was a problem hiding this comment.
Thanks for the PR. It works nicely. A few comments about the grammar of the output messages, but looks good apart from that.
| 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.") |
There was a problem hiding this 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.".
There was a problem hiding this comment.
Alternatively, and this is what they do elsewhere in this tool, use ${entityType.dropRight(1)}.
| 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."); |
There was a problem hiding this 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.
Signed-off-by: PoAn Yang <payang@apache.org>
|
not sure if I overlooked the vote, but the vote does not pass yet (https://lists.apache.org/thread/bmr5l38gfnqrws3qfpxqfp7y9r7lzdrc). Is there another vote thread? |
Sorry that I checked on the wrong thread :( |
|
I think there are two threads: |
4 participants
non-existent resource in kafka-configs.sh and kafka-client-metrics.sh.
non-existent groups but having dynamic config. If it cannot find a group
in both conditions, return resource doesn't exist message.
Reviewers: Lan Ding 53332773+DL1231@users.noreply.github.com, Andrew
Schofield aschofield@confluent.io