Hi Colin, Thank you for your feedback! Please find my comments/answers below:
*> 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..."* That's fair. I will update it. *> 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.* Good point. I like your suggestion. I will update it. *> 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.* Sorry for the confusion. I think my usage of the word "add" is not appropriate here. The ApiVersionsResponse won't change as you said. My proposal is to provide the supported versions of the ApiVersionsRequest in the response by populating the existing `api_versions` field. In the current version, when an error occurs, the `api_versions` is empty in the response. Providing it enables the client to re-send the latest version supported by the broker instead of defaulting to zero. I will update the KIP to make this clearer. *> 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.* Good point. I will use `Version 3 is the same as version 2.` which is the statement already used in other requests/responses. *> 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.* Right. I have put it because I thought your PR would do it. I will update this. By the way, it means that the request/response must be updated to the generated ones, isn't it? AVR is still using the old mechanism. *> 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.* I do agree and I have already put something along those lines in the proposal. See the "Validation" chapter. I have proposed to use a more restrictive validation which does not allow white spaces. I think spaces wouldn't be used in software name nor version. Is it OK for you if we stick to the more restrictive one? Thank your letting me know about the underscores. I have missed this. Regarding disconnecting when the validation fails, this is what I have proposed as well. Magnus has brought a good point though. Using an explicit error like `INVALID_REQUEST` may be better. In this case, the client would have to disconnect when it happens. I will update the KIP to reflect this. Best, David On Wed, Sep 18, 2019 at 9:46 PM Colin McCabe <cmcc...@apache.org> wrote: > 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 > > > > > > > > > >
