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