Hi Kirk, Thanks for this thoughtful kip.
I have a few additional points. mb-1 : Would there be any recommendation for reference implementation to store the client configs ? may be in a compacted topic etc. Otherwise, everyone will come up with their impl, and could be re-invented again and again? mb-2 : When a client reconnects, there might be duplicates of clientconfigs pushed. Should this be handled by plugin may be based on uuid/clientid? (I am not sure, if client restarts and there is a new uuid, would there be stale entries indefinitely) mb-3 : In your reply to Hector, you noted "it's difficult to ensure that client is marked as 'in violation' across all brokers in a cluster." Does the same challenge apply to the observability goal? If the PushConfig lands on a single randomly chosen broker, only that broker's ClientConfigPolicy sees it, so the plugin effectively must fan out to shared storage for the feature to be useful cluster-wide ? mb-4 : If AdminClient configs are not handled, how about any dynamic config changes like AdminClient.alterClientConfigs would also work like hot reload ? Or any explicit re-push mechanism be possible? mb-5 : Regarding configs.push.allowed.keys, would it help if there is a deny list which can override allow list? or to avoid any sensitive keys just in case. mb-6 : In general, if the rpc fails for one random node, will it retry to a different node? Thanks, Murali On Sun, May 3, 2026 at 1:37 PM Kirk True <[email protected]> wrote: > 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 > > > > > >
