That's fair. We could use the existing error code in the response and pass back something like INVALID_REQUEST.
I'm not sure if we want to add an error string field just for this (although they're a good idea in general...) best, Colin On Wed, Sep 18, 2019, at 12:31, Magnus Edenhill wrote: > > I think we should force client software names and versions to follow a > regular expression and disconnect if they do not. > > Disconnecting is not really a great error propagation method since it > leaves the client oblivious to what went wrong. > Instead suggest we return an ApiVersionResponse with an error code and a > human-readable error message field. > > > > Den ons 18 sep. 2019 kl 20:05 skrev Colin McCabe <cmcc...@apache.org>: > > > 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 > > > > > >
