-
Notifications
You must be signed in to change notification settings - Fork 14.4k
KAFKA-18904: kafka-configs.sh return resource doesn't exist message [3/N] #19808
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
…urce doesn't exist message Signed-off-by: PoAn Yang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FrankYang0529 Thanks for the PR, overall LGTM.
.map(name -> new ConfigResource(entry.getKey(), name))) | ||
.collect(Collectors.toList()); | ||
return new ListConfigResourcesResult(KafkaFuture.completedFuture(resources)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra blank
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can't this just be util.Set.of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to leave this as java.util.xxx
because we already used this pattern like:
val alterEntityMap = new java.util.HashMap[String, String] |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Signed-off-by: PoAn Yang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.