You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jun Rao <ju...@confluent.io> on 2019/10/11 20:49:09 UTC
Re: [VOTE] KIP-511: Collect and Expose Client's Name and Version in
the Brokers
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 <dj...@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 <dj...@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...@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 <
> > >> > konstantine@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 <gw...@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 <
> cmccabe@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 <
> > >> cmccabe@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 <
> > >> > cmccabe@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 <
> > >> > > > cmccabe@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 <
> > >> > > > > > > djacot@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
> > >> > > > > > > > > > > >
> > >> > > > > > > > > > >
> > >> > > > > > > > > >
> > >> > > > > > > > >
> > >> > > > > > > >
> > >> > > > > > >
> > >> > > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> >
>
Re: [VOTE] KIP-511: Collect and Expose Client's Name and Version in
the Brokers
Posted by David Jacot <dj...@confluent.io>.
Hi all,
We have completed the implementation of the KIP. During the implementation
of the second part, we have reworked the metrics. We have ended up with
only one metric which represents the number of connections using a given
client software name and version. We have made it per selector as it
simplifies the concurrency and do not require locking. It looks as follows:
```
kafka.server:clientSoftwareName=(client-software-name),clientSoftwareVersion=(client-software-version),listener=(listener),networkProcessor=(processor-index),type=(type)
```
I have updated the KIP to reflect this change. Please, let us know if you
have any objections.
Best,
David
On Mon, Oct 14, 2019 at 4:42 PM Xu Jianhai <sn...@gmail.com> wrote:
> +1
>
> On Mon, Oct 14, 2019 at 3:47 PM David Jacot <dj...@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 <wa...@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 <ju...@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 <
> jason@confluent.io
> > >
> > > > > wrote:
> > > > >
> > > > > > Thanks David for the clarification. That sounds good.
> > > > > >
> > > > > > On Mon, Sep 23, 2019 at 12:35 AM David Jacot <
> djacot@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 <
> djacot@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.kevin@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 <
> > > > jason@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 <
> > > > > > > >> > konstantine@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 <
> > > > gwen@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 <
> > > > > > cmccabe@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 <
> > > > > > > >> cmccabe@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 <
> > > > > > > >> > cmccabe@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 <
> > > > > > > >> > > > cmccabe@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 <
> > > > > > > >> > > > > > > djacot@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
> > > >
> > >
> >
>
Re: [VOTE] KIP-511: Collect and Expose Client's Name and Version in
the Brokers
Posted by Xu Jianhai <sn...@gmail.com>.
+1
On Mon, Oct 14, 2019 at 3:47 PM David Jacot <dj...@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 <wa...@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 <ju...@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 <jason@confluent.io
> >
> > > > wrote:
> > > >
> > > > > Thanks David for the clarification. That sounds good.
> > > > >
> > > > > On Mon, Sep 23, 2019 at 12:35 AM David Jacot <dj...@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 <djacot@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.kevin@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 <
> > > jason@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 <
> > > > > > >> > konstantine@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 <
> > > gwen@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 <
> > > > > cmccabe@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 <
> > > > > > >> cmccabe@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 <
> > > > > > >> > cmccabe@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 <
> > > > > > >> > > > cmccabe@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 <
> > > > > > >> > > > > > > djacot@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
> > >
> >
>
Re: [VOTE] KIP-511: Collect and Expose Client's Name and Version in
the Brokers
Posted by David Jacot <dj...@confluent.io>.
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 <wa...@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 <ju...@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 <dj...@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 <dj...@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...@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 <
> > jason@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 <
> > > > > >> > konstantine@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 <
> > gwen@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 <
> > > > cmccabe@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 <
> > > > > >> cmccabe@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 <
> > > > > >> > cmccabe@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 <
> > > > > >> > > > cmccabe@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 <
> > > > > >> > > > > > > djacot@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
> >
>
Re: [VOTE] KIP-511: Collect and Expose Client's Name and Version in
the Brokers
Posted by Harsha Chintalapani <ka...@harsha.io>.
+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 <wa...@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 <ju...@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 <dj...@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 <dj...@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...@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 <
> jason@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 <
> > > > >> > konstantine@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 <
> gwen@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 <
> > > cmccabe@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 <
> > > > >> cmccabe@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 <
> > > > >> > cmccabe@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 <
> > > > >> > > > cmccabe@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 <
> > > > >> > > > > > > djacot@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
>
Re: [VOTE] KIP-511: Collect and Expose Client's Name and Version in
the Brokers
Posted by Guozhang Wang <wa...@gmail.com>.
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 <ju...@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 <dj...@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 <dj...@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...@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 <jason@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 <
> > > >> > konstantine@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 <gwen@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 <
> > cmccabe@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 <
> > > >> cmccabe@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 <
> > > >> > cmccabe@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 <
> > > >> > > > cmccabe@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 <
> > > >> > > > > > > djacot@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