Hi Andrew, On Tue, Apr 28, 2026, at 6:28 PM, Andrew Schofield wrote: > Hi Kirk, > Thanks for the KIP. Inevitably, I have some comments. > > AS1: I see a few mysterious mentions of "profile". I suppose that these are > evidence of a concept which did not eventually see the light of day in the > KIP.
That's a vestige from a previous design. I will remove them ASAP. > AS2: It seems that the default configurations pushed for a share consumer are > incorrect (because some of them are group configs not client configs). I > suggest: > > * client.id > * group.id > * share.acknowledgement.mode > * share.acquire.mode > * max.poll.records > * max.poll.interval.ms > * fetch.min.bytes > * fetch.max.wait.ms Thanks for the feedback! > AS3: Is there any reason why you need to send the configuration type > information to the broker? I wonder if it would be simpler just to leave > everything as strings. I see that you've got a enum for ClientConfigType > which is awfully similar to Config.Type, and you're going to end up mapping > between these enums. Most of the configuration we're collecting by default end up being numeric, so defaulting to strings is less efficient. Also, the thought process was that implementors of the ClientConfigPolicy plugin may benefit from/need the type information to achieve their goal. Agreed that the mapping between ClientConfigType and Config.Type seems unnecessary. ClientConfigType is a strict subset of Config.Type, specifically no PASSWORD type, so I wanted to provide some compile time support to prevent their usage. Maybe I was being too zealous in this regard? > AS4: ClientConfigUnknownProfileException has a couple of problems. We tend to > use Unknown at the start of the error codes and exception class names, not > the middle. Profile is not a thing. I suggest it should be > UnknownClientConfigSomethingException, but I am not qualified to say what the > Something is. I will propose a change in the next revision of the KIP. Thanks for catching that. > AS5: You should include a list of the exceptions and error codes you are > introducing to the protocol. I've seen a few new exceptions and they > generally have error codes which correspond 1:1. > > * ClientConfigUnknownProfileException - Should this just be > InvalidConfigurationException (exists) which maps to INVALID_CONFIG error > code? > * ClientConfigTooLargeException - I would expect this to map to the > CLIENT_CONFIG_TOO_LARGE error code (new) > * ClientConfigPolicyException - See AS4. Policy exceptions are usually > PolicyViolationException, but this policy doesn't validate, it just processes. I wasn't certain where on the specificity vs. generality spectrum to land on creating or reusing existing errors. In my next KIP revision I'll look to reuse existing error codes where possible. > AS6: You've tended to use Config (singular) not Configs (plural). However, in > the configurations for the configurations, you've used plural, such as > client.configs.policy.class.name. I would err on the side of consistency. Agreed. I'll make this more consistent. > AS7: We are going to have to resolve the relationship between this KIP and > KIP-1313. The latter introduces client instance ID on all RPCs, and will > simplify your KIP if it is accepted first. Yes, KIP-1313 are racing in this regard. I see KIP-1313 has entered the voting phase, so I will likely end up removing most of the client ID instance references from my KIP, leaving on those that are more pertinent to the KIP's specific needs. > AS8: Please confirm whether there are any timing considerations between > PushConfigs and GetTelemetrySubscriptions/PushTelemetry RPCs. I suppose that > a client could overlap these RPCs, or even send them to different randomly > selected brokers, as part of its initial connection setup. The intention was for the PushConfig RPC to execute before the telemetry RPCs. However, this will likely change based on other feedback, moving toward more of an "overlap" sequencing. From the perspective of this KIP, it shouldn't matter if they're sent to different brokers. I wonder if the ClientConfigPolicy plugin implementors would see that differently? > AS9: Please add some broker metrics for this new feature. I suggest looking > at KIP-714 for inspiration. Will do. I intentionally omitted metrics on the broker, leaving them for the specific implementations. But I will look at the KIP-714 metrics, as suggested. > AS10: Why does the client block while the config handshake is being > performed? The handshake is not validating the configurations and the client > doesn't throw an exception even if the PushConfigs response contains an error > code. Doesn't this unnecessarily slow down the initial connection, and that's > arguably too long with Kafka clients already. The approach will likely move away from a blocking approach to a background, best effort approach. I will update the KIP to reflect this. > AS11: Which are the default configurations sent for the Apache Kafka Java > admin client? In initial discussions, the admin client was deemed as too uninteresting to warrant sending configuration since it doesn't effect correctness or performance of the critical produce/consume path. Do you disagree? Thanks for the thorough feedback! Kirk > Thanks, > Andrew > > On 2026/04/23 17:59:11 Kirk True wrote: > > Hi all, > > > > I would like to start a discussion on KIP-1324: Support client > > configuration observability: > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1324%3A+Support+client+configuration+observability > > > > Thanks, > > Kirk > > >
