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 >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > >> > > >> > >> >
