+1 (non-binding) Definitely needed this before as it would have saved me some time from trying to guess a client's version from api version/source code. Thanks for the KIP!
Regards, Kevin On Fri, Sep 20, 2019 at 4:29 PM Jason Gustafson <ja...@confluent.io> wrote: > +1 from me. This is a clever solution. Kind of a pity we couldn't work > flexible version support into the response, but I understand why it is > difficult. > > One minor nitpick: the INVALID_REQUEST error already exists. Are you > intending to reuse it? > > Thanks, > Jason > > On Fri, Sep 20, 2019 at 3:50 PM Konstantine Karantasis < > konstant...@confluent.io> wrote: > > > Quite useful KIP from an operational standpoint and I also like it in its > > most recent revised form. > > > > +1 (non-binding). > > > > Konstantine > > > > > > On Fri, Sep 20, 2019 at 9:55 AM Gwen Shapira <g...@confluent.io> wrote: > > > > > +1 (binding) > > > > > > Thanks for the KIP, David and for the help with the design, Colin. I > > > think it looks great now. > > > > > > On Fri, Sep 20, 2019 at 9:42 AM Colin McCabe <cmcc...@apache.org> > wrote: > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
