Hi all, Jun, The new fields are not flexible fields while the request and response are flexible versions. It does not cost us because the version of the request is bumped anyway to enable the flexible versions. The rationale behind this choice is that it forces the clients to provide the information.
Guozhang, "Connections" is the name of the metric and it does not change. Concerns regarding the scalability of this metric have been expressed during the implementation so we won't implement it. The idea was to allow operators to list all the active connections via JMX. I am looking for alternatives at this point. Best, David On Sat, Oct 12, 2019 at 1:15 AM Harsha Chintalapani <ka...@harsha.io> wrote: > +1 (binding). Thanks for the KIP this is going to be super useful. > > Thanks, > Harsha > > On Fri, Oct 11, 2019 at 2:57 PM Guozhang Wang <wangg...@gmail.com> wrote: > > > Hi David, > > > > Thanks for the KIP. I know I'm late on voting here but I'd be +1 on this > > too! > > > > Just a quick clarification on the tag "name=Connections" of the third > > metric, what would be the format of "Connections" here? Would that be the > > connection listener name? > > > > > > Guozhang > > > > > > On Fri, Oct 11, 2019 at 1:49 PM Jun Rao <j...@confluent.io> wrote: > > > > > 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 > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > -- > > -- Guozhang > > >
