Couple of missing things:
This KIP doesn't have a proposal on versioning it just gives different
options, it'd be good to get a concrete proposal in the KIP. Here is my
understanding of what we are proposing (can someone sanity check and if
correct, update the kip):
1. We will augment the existing api_version field in the header with a
protocol_version that will begin at some initial value and increment by 1
every time we make a changes to any of the api_versions (question:
including internal apis?).
2. The protocol_version will be added to the metadata request
3. We will also add a string that this proposal is calling VersionString
which will describe the build of kafka in some way. The clients should not
under any circumstances do anything with this string other than print it
out to the user.
One thing I'm not sure about: I think currently metadata sits in the client
for 10 mins by default. Say a client bootstraps and then a server is
downgraded to an earlier version, won't the client's metadata version
indicate that that client handles a version it doesn't actually handle any
more? We need to document how clients will handle this.
Here are some comments on other details:
1. As a minor thing I think we should avoid naming the fields VersionId
and VersionString which sort of implies they are both used for versioning.
I think we should call them something like ProtocolVersion and
BuildDescription, with BuildDescription being totally unspecified other
than that it is some kind of human readable string describing a particular
Kafka build. We really don't want a client attempting to use this string in
any way as that would always be the wrong thing to do in the versioning
scheme we are proposing, you should always use the protocol version.
2. Does making the topics field in the metadata request nullable
actually make sense? We have a history of wanting to add magical values
rather than fields. Currently topics=[a] means give me information about
topic a, topics=[] means give me information about all topics, and we are
proposing topics=null would mean don't give me topics. I don't have a
strong opinion.
3. I prefer Jason's proposal on using a conservative metadata version
versus the empty response hack. However I think that may actually
exacerbate the downgrade scenario I described.
4. I agree with Jason that we should really look at the details of the
implementation so we know it works--implementing server support without
actually trying it is kind of risky.
As a meta comment: I'd really like to encourage us to think of the protocol
as a document that includes the following things:
- The binary format, error codes, etc
- The request/response interaction
- The semantics of each request in different cases
- Instructions on how to use this to implement a client
This document is versioned with the protocol number and is the source of
truth for the protocol.
Part of any protocol change needs to be an update to the instructions on
how to use that part of the protocol. We should be opinionated. If there
are two options there should be a reason, and then we need to document both
and say exactly when to use each.
I think we also need to get a "how to" document on protocol changes just so
people know what they need to do to add a new protocol feature.
-Jay
On Mon, Mar 14, 2016 at 4:51 PM, Jason Gustafson <ja...@confluent.io> wrote:
> >
> > Are you suggesting this as a solution for the problem where a KIP-35
> aware
> > client sends a higher version of metadata req, say v2, to a KIP-35 aware
> > broker that only supports up to v1.
>
>
> Yes, that's right. In that case, the client first sends v1, finds out that
> the broker supports v2, and then sends v2 (if it has any reason to do so).
>
> We had a bit of discussion on such scenarios, and it seemed to be a chicken
> > and egg problem that is hard to avoid. Your suggestion definitely makes
> > sense, however it falls under the purview of clients.
>
>
> That basically means clients should figure it out for themselves? Might be
> nice to have a better answer.
>
> KIP-35 only aims on adding support for getting the version info from a
> > broker. This definitely can be utilized by our clients. However, that can
> > follow KIP-35 changes. Does this sound reasonable to you?
>
>
> It may be OK, but I'm a little concerned about offering a feature that we
> don't support ourselves. Sometimes it's not until implementation that we
> find out whether it really works as expected. And if we're eventually
> planning to support it, I feel we should think through some of the cases a
> bit more. For example, the upgrade and downgrade cases that Becket
> mentioned earlier. It doesn't feel too great to support this feature unless
> we can offer guidance on how to use it.
>
> -Jason
>
>
>
> On Mon, Mar 14, 2016 at 4:20 PM, Ashish Singh <asi...@cloudera.com> wrote:
>
> > Hi Jason,
> >
> > On Mon, Mar 14, 2016 at 4:04 PM, Jason Gustafson <ja...@confluent.io>
> > wrote:
> >
> > > Perhaps clients should always send the oldest version of the metadata
> > > request which supports KIP-35 when initially connecting to the cluster.
> > > Depending on the versions in the response, it can upgrade to a more
> > recent
> > > version. Then maybe we don't need the empty response hack?
> > >
> > Are you suggesting this as a solution for the problem where a KIP-35
> aware
> > client sends a higher version of metadata req, say v2, to a KIP-35 aware
> > broker that only supports up to v1.
> >
> > We had a bit of discussion on such scenarios, and it seemed to be a
> chicken
> > and egg problem that is hard to avoid. Your suggestion definitely makes
> > sense, however it falls under the purview of clients.
> >
> > >
> > > One thing that's not clear to me is whether the ultimate goal of this
> KIP
> > > is to have our clients support multiple broker versions. It would be a
> > > little weird to have this feature if our own clients don't use it.
> > >
> > KIP-35 only aims on adding support for getting the version info from a
> > broker. This definitely can be utilized by our clients. However, that can
> > follow KIP-35 changes. Does this sound reasonable to you?
> >
> > >
> > > -Jason
> > >
> > > On Mon, Mar 14, 2016 at 3:34 PM, Ashish Singh <asi...@cloudera.com>
> > wrote:
> > >
> > > > On Mon, Mar 14, 2016 at 2:12 PM, Ismael Juma <ism...@juma.me.uk>
> > wrote:
> > > >
> > > > > On Mon, Mar 14, 2016 at 8:45 PM, Gwen Shapira <g...@confluent.io>
> > > wrote:
> > > > > >
> > > > > > > I don't see how it helps. If the client is communicating with a
> > > > broker
> > > > > > that
> > > > > > > does not support KIP-35, that broker will simply close the
> > > > connection.
> > > > > If
> > > > > > > the broker supports KIP-35, then it will provide the broker
> > > version.
> > > > I
> > > > > > > don't envisage a scenario where a broker does not support
> KIP-35,
> > > but
> > > > > > > implements the new behaviour of sending an empty response. Do
> > you?
> > > > > > >
> > > > > > > Are you sure about that? Per KIP-35, the broker supplies the
> > > version
> > > > in
> > > > > > response to Metadata request, not in response to anything else.
> > > > > > If the client sends producer request version 42 (accidentally or
> > due
> > > to
> > > > > > premature upgrade) to KIP-35-compactible broker - we want to see
> an
> > > > empty
> > > > > > packet and not a connection close.
> > > > > > Sending a broker version was deemed impractical IIRC.
> > > > > >
> > > > >
> > > > > OK, so this is a different case than the one Ashish described
> > ("client
> > > > that
> > > > > wants to support broker versions that do not provide broker version
> > in
> > > > > metadata and broker versions that provides version info in
> > metadata").
> > > > So,
> > > > > you are suggesting that if a client is communicating with a broker
> > that
> > > > > implements KIP-35 and it receives an empty response, it will assume
> > > that
> > > > > the broker doesn't support the request version and it won't try to
> > > parse
> > > > > the response? I think it would be good to explain this kind of
> thing
> > in
> > > > > detail in the KIP.
> > > > >
> > > > Actually even in this case and the case I mentioned, closing
> connection
> > > > should be fine. Lets think about possible reasons that could lead to
> > this
> > > > issue.
> > > >
> > > > 1. Client has incorrect mapping of supported protocols for a broker
> > > > version.
> > > > 2. Client misread broker version from metadata response.
> > > > 3. Client constructed unsupported protocol version by mistake.
> > > >
> > > > In all the above cases irrespective of what broker does, client will
> > keep
> > > > sending wrong request version.
> > > >
> > > > At this point, I think sending an empty packet instead of closing
> > > > connection is a nice to have and not mandatory requirement. Like in
> the
> > > > above case, a client can catch parsing error and be sure that there
> is
> > > > something wrong in the protocol version it is sending. However, a
> > generic
> > > > connection close does not really provide any information on probable
> > > cause.
> > > >
> > > > What do you guys suggest?
> > > >
> > > > >
> > > > > Ismael
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > >
> > > > Regards,
> > > > Ashish
> > > >
> > >
> >
> >
> >
> > --
> >
> > Regards,
> > Ashish
> >
>
