+1 On Mon, Oct 14, 2019 at 3:47 PM David Jacot <dja...@confluent.io> wrote:
> 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 > > > > > >
