Hi, David, Thanks for the KIP. Just a minor comment below.
100. It seems that the new flexible fields need tag numbers. Could you add them to the wiki? Jun On Mon, Sep 23, 2019 at 11:37 AM Jason Gustafson <ja...@confluent.io> wrote: > Thanks David for the clarification. That sounds good. > > On Mon, Sep 23, 2019 at 12:35 AM David Jacot <dja...@confluent.io> wrote: > > > Hi all, > > > > The vote has passed with +3 binding votes (Colin McCabe, Gwen Shapira, > > Jason Gustafson) and +3 non binding votes (Mickael Maison, Konstantine > > Karantasis, Kevin Lu). \o/ > > > > Thanks to everyone that reviewed and helped improve this proposal, and > > huge thanks to Colin for his great feedback. > > > > Best, > > David > > > > On Mon, Sep 23, 2019 at 9:28 AM David Jacot <dja...@confluent.io> wrote: > > > > > Hi Jason, > > > > > > The response will be a flexible version but the response header won't > be > > > (only for the api versions response). I have forgotten to change this > > point > > > in the KIP. I will make this point clearer. > > > > > > I didn't know that INVALID_REQUEST already exists. Yes, it makes sense > to > > > reuse it then. > > > > > > Best, > > > David > > > > > > On Sat, Sep 21, 2019 at 3:02 AM Kevin Lu <lu.ke...@berkeley.edu> > wrote: > > > > > >> +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 > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > >
