On Fri, Sep 20, 2019, at 01:45, David Jacot wrote: > > Thanks for the clarification. The proposed behavior sounds reasonable. > > Can you add a note about the implementation on the client? The client > > needs to be prepared to handle > a response that doesn't include the > > versions, as well, since v1 did not. > > I have added a note about the implementation in the KIP. In short, when the > client receives an unsupported version, it defaults to version 0. As > version 0 contains both the ErrorCode and the ApiKeys fields, the client > can check the error and in case of UNSUPPORTED_VERSION, it can check the > ApiKeys to get the supported versions. If not present, it default to > version 0. > > > Hmm. Like we discussed above, there is a very important difference in the > > v3 response, which is that the versions will be included even if the > > client's version was higher than what the broker supports. > > We should add a comment about that. > > Yeah. I think the change that we propose to enhance the handling of > unsupported versions of ApiVersionsRequest/Response is orthogonal to the > version bump. Concretely, the versions will be included in the > ApiVersionsResponse v0 - the request/response used by the broker when > failing back - so it is a bit misleading to say that starting from version > 3, the broker populate the ApiKeys field with the information about the > supported versions of the AVR. I would rather put a note saying: Starting > from Apache Kafka 2.4, ApiKeys field is populated with the supported > versions of the ApiVersionsRequest when an UNSUPPORTED_VERSION error is > returned. Would this work for you?
Thanks for the clarification. Yes, that makes sense. Adding the additional fields doesn't need to be tied to the version of ApiVersionsResponse. Keep in mind, though, that you still have to handle responses from older brokers, which will not include this information. I assume that you will distinguish those responses based on the length of the response. We should add this detail to the KIP. +1 (binding) after that change. cheers, Colin > > > Agreed. This is a good use-case for INVALID_REQUEST. We should add a > comment that this is now a valid error. > > I have documented the error in the Public Interfaces section. > > Best, > David > > On Thu, Sep 19, 2019 at 10:52 PM Colin McCabe <cmcc...@apache.org> wrote: > > > On Wed, Sep 18, 2019, at 23:44, David Jacot 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. > > > > Thanks for the clarification. The proposed behavior sounds reasonable. > > Can you add a note about the implementation on the client? The client > > needs to be prepared to handle a response that doesn't include the > > versions, as well, since v1 did not. > > > > > > > > *> 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. > > > > Hmm. Like we discussed above, there is a very important difference in the > > v3 response, which is that the versions will be included even if the > > client's version was higher than what the broker supports. We should add a > > comment about that. > > > > > > > > > > > *> 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. > > > > Yeah, I think we should move to the new mechanism. It should be very easy > > for the request. The response may be slightly more difficult, but probably > > not that much more. > > > > > > > > > > > > > > *> 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. > > > > Yeah, the one you proposed sounds fine. > > > > > > > > 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. > > > > Agreed. This is a good use-case for INVALID_REQUEST. We should add a > > comment that this is now a valid error. > > > > best, > > Colin > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > >
