You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by David Jacot <dj...@confluent.io> on 2019/12/16 09:32:47 UTC
Re: [VOTE] KIP-511: Collect and Expose Client's Name and Version in
the Brokers
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
> > > >
> > >
> >
>