KAFKA-17516: Synonyms for client metrics configs #17264
Conversation
| val metricResource = new ConfigResource(ConfigResource.Type.CLIENT_METRICS, "none_metric") | ||
| val metricResult = client.describeConfigs(Seq(metricResource).asJava).all().get().get(metricResource) | ||
| assertEquals(0, metricResult.entries().size()) |
There was a problem hiding this comment.
Why do need to remove this line? Is it because now default configs will always appear for unknown subscription? Will it add confusion when describing? Should we just add a note in describe that if subscription doesn not exist then default values will be displayed, and check for subscriptions list to find which all subscriptions are applied to the cluster.
There was a problem hiding this comment.
Well, yes, that's what happens. Previously, if you displayed a non-existent resource, it didn't say "This doesn't exist", it just printed an empty list. Now, it prints the defaults. What ideally would happen is that it would say "This resource doesn't exist." but I think that's tricky to achieve. If you have ideas here, let me know. I'd prefer if the resource was flagged as an error in this case but the client metrics config manager in the broker is a tricky beast I think.
There was a problem hiding this comment.
I've dug more into this. The behaviour is the same for groups and client metrics. If you describe a non-existent configs resource, it returns an empty Properties() (this is in ConfigurationsImage.configProperties(ConfigResource). Then, the defaults are applied to this empty set which makes it look like the config resource exists.
I think it's worth making this work properly such that describing an existing resource works, and a non-existent resource fails neatly.
There was a problem hiding this comment.
I agree shall we do it as part of this PR or shall take it separtely? Whatever works with you.
There was a problem hiding this comment.
Having taken a look at the code in more detail, I would prefer to do it as a follow-on PR. This PR would grow quite a bit doing both things at once.
There was a problem hiding this comment.
I expected as well, thanks a lot @AndrewJSchofield for the fix and additional work.
|
@chia7712 I wonder if you'd be able to take a look at this PR since I know you have reviewed PRs in this area before. Thanks. |
|
@AndrewJSchofield could you please fix the conflicts? I will take a look later |
|
@chia7712 Would you be able to give me a review on this PR please? |
This PR brings client metrics configuration resources in line with the other config resources in terms of handling synonyms and defaults. Specifically, configs which are not explicitly set take their hard-coded default values, and these are reported by
kafka-configs.sh --describeandKafka-client-metrics.sh --describe. Previously, they were omitted which means the administrator needed to know the default values.The ConfigHelper was changed so that the handling of client metrics configuration matches that of group configuration.
Committer Checklist (excluded from commit message)