Hi David, Thanks for the KIP!
Nitpick: in the intro paragraph, "Operators of Apache Kafka clusters have literally no information about the clients connected to their clusters" seems a bit too strong. We have some information, right? For example, the client ID, where clients are connecting from, etc. Maybe it would be clearer to say "very little information about the type of client software..." Instead of ClientName and ClientVersion, how about ClientSoftwareName and ClientSoftwareVersion? This might make it clearer what the fields are for. I can see people getting confused about the difference between ClientName and ClientId, which sound pretty similar. Adding "Software" to the field name makes it much clearer what the difference is between these fields. In the "ApiVersions Request/Response Handling" section, it seems like there is some out-of-date text. Specifically, it says "we propose to add the supported version of the ApiVersionsRequest in the response sent back to the client alongside the error...". But on the other hand, elsewhere in the KIP, we say "ApiVersionsResponse is bumped to version 3 but does not have any changes in the schema" Based on the offline discussion we had, the correct text is the latter (we're not changing ApiVersionsRerponse). So we should remove the text about adding stuff to ApiVersionsResponse. In a similar vein, the comment " // Version 3 is similar to version 2" should be " // Version 3 is identical to version 2" or something like that. Although I guess technically things which are identical are also similar, the current phrasing could be misleading. Now that KIP-482 has been accepted, I think there are a few things that are worth clarifying in the KIP. Firstly, ApiVersionsRequest v3 should be a "flexible version". Mainly, that means its request header will support optional tagged fields. However, ApiVersionsResponse v3 will *not* support optional tagged fields in its response header. This is necessary because-- as you said-- the broker must look at a fixed offset to find the error code, regardless of the response version. I think we should force client software names and versions to follow a regular expression and disconnect if they do not. This will prevent issues when using these strings in metrics names. Probably we want something like: [\.\-A-Za-z0-9]?[\.\-A-Za-z0-9 ]*[\.\-A-Za-z0-9]? Notice this does _not* include underscores, since they get converted to dots in JMX, causing ambiguity. It also doesn't allow the first or last character to be a space. best, Colin On Mon, Sep 16, 2019, at 04:39, Mickael Maison wrote: > +1 (non binding) > Thanks for the KIP! > > On Mon, Sep 16, 2019 at 12:07 PM David Jacot <dja...@confluent.io> wrote: > > > > Hi all, > > > > I would like to start a vote on KIP-511: > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-511%3A+Collect+and+Expose+Client%27s+Name+and+Version+in+the+Brokers > > > > Best, > > David >
