Hi all, I have updated the KIP to incorporate Colin's feedback.
Best, David On Thu, Sep 19, 2019 at 8:44 AM David Jacot <dja...@confluent.io> wrote: > 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 >> > > > >> > > >> > >> >
