But won't it be the case that what clients end up doing would be something
like
if(version != 0.8.1)
throw new UnsupportedVersionException()
which then means the client is broken as soon as we release a new server
version even though the protocol didn't change. I'm actually not sure how
you could use that information in a forward compatible way since you can't
know a priori if you will work with the next release until you know if the
protocol changed.
-Jay
On Wed, Mar 2, 2016 at 5:28 PM, Jason Gustafson <ja...@confluent.io> wrote:
> Hey Jay,
>
> Yeah, I wasn't suggesting that we eliminate request API versions. They're
> definitely needed on the broker to support compatibility. I was just saying
> that if a client wants to support multiple broker versions (e.g. 0.8 and
> 0.9), then it makes more sense to me to make the kafka release version
> available in order to determine which version of the request API should be
> used rather than adding a new request type which exposes all of the
> different supported versions for all of the request types. Request API
> versions all change in lockstep with Kafka releases anyway.
>
> -Jason
>
> On Wed, Mar 2, 2016 at 5:15 PM, Becket Qin <becket....@gmail.com> wrote:
>
> > I think using Kafka release version makes sense. More particularly, we
> can
> > use the ApiVersion and this will cover all the interval version as well.
> In
> > KAFKA-3025, we added the ApiVersion to message format version mapping, We
> > can add the ApiKey to version mapping to ApiVersion as well. We can move
> > ApiVersion class to o.a.k.c package and use it for both server and
> clients.
> >
> > @Jason, if we cache the release info in metadata and not re-validate the
> > release on reconnect, would it still work if we do a rolling downgrade?
> >
> > On Wed, Mar 2, 2016 at 3:16 PM, Jason Gustafson <ja...@confluent.io>
> > wrote:
> >
> > > I think Dana's suggestion to include the Kafka release version makes a
> > lot
> > > of sense. I'm actually wondering why you would need the individual API
> > > versions if you have that? It sounds like keeping track of all the api
> > > version information would add a lot of complexity to clients since
> > they'll
> > > have to try to handle different version permutations which are not
> > actually
> > > possible in practice. Wouldn't it be simpler to know that you're
> talking
> > to
> > > an 0.9 broker than that you're talking to a broker which supports
> > version 2
> > > of the group coordinator request, version 1 of fetch request, etc?
> Also,
> > > the release version could be included in the broker information in the
> > > topic metadata request which would save the need for the additional
> round
> > > trip on every reconnect.
> > >
> > > -Jason
> > >
> > > On Tue, Mar 1, 2016 at 7:59 PM, Ashish Singh <asi...@cloudera.com>
> > wrote:
> > >
> > > > On Tue, Mar 1, 2016 at 6:30 PM, Gwen Shapira <g...@confluent.io>
> > wrote:
> > > >
> > > > > One more thing, the KIP actually had 3 parts:
> > > > > 1. The version protocol
> > > > > 2. New response on messages of wrong API key or wrong version
> > > > > 3. Protocol documentation
> > > > >
> > > > There is a WIP patch for adding protocol docs,
> > > > https://github.com/apache/kafka/pull/970 . By protocol
> documentation,
> > > you
> > > > mean updating this, right?
> > > >
> > > > >
> > > > > I understand that you are offering to only implement part 1?
> > > > > But the KIP discussion and vote should still cover all three parts,
> > > > > they will just be implemented in separate JIRA?
> > > > >
> > > > The patch for KAFKA-3307, https://github.com/apache/kafka/pull/986,
> > > covers
> > > > 1 and 2. KAFKA-3309 tracks documentation part. Yes, we should include
> > all
> > > > the three points you mentioned while discussing or voting for KIP-35.
> > > >
> > > > >
> > > > > On Tue, Mar 1, 2016 at 5:25 PM, Ashish Singh <asi...@cloudera.com>
> > > > wrote:
> > > > > > On Tue, Mar 1, 2016 at 5:11 PM, Gwen Shapira <g...@confluent.io>
> > > > wrote:
> > > > > >
> > > > > >> I don't see a use for the name - clients should be able to
> > translate
> > > > > >> ApiKey to name for any API they support, and I'm not sure why
> > would
> > > a
> > > > > >> client need to log anything about APIs it does not support. Am I
> > > > > >> missing something?
> > > > > >>
> > > > > > Yea, it is a fair assumption that client would know about APIs it
> > > > > supports.
> > > > > > It could have been helpful for client users to see new APIs
> though,
> > > > > however
> > > > > > users can always refer to protocol doc of new version to find
> > > > > corresponding
> > > > > > names of the new APIs.
> > > > > >
> > > > > >>
> > > > > >> On a related note, Magnus is currently on vacation, but he
> should
> > be
> > > > > >> back at the end of next week. I'd like to hold off on the vote
> > until
> > > > > >> he gets back since his experience in implementing clients and
> his
> > > > > >> opinions will be very valuable for this discussion.
> > > > > >>
> > > > > > That is great. It will be valuable to have his feedback. I will
> > hold
> > > > off
> > > > > on
> > > > > > removing "api_name" and "api_deprecated_versions" or adding
> release
> > > > > version.
> > > > > >
> > > > > >>
> > > > > >> Gwen
> > > > > >>
> > > > > >> On Tue, Mar 1, 2016 at 4:24 PM, Ashish Singh <
> asi...@cloudera.com
> > >
> > > > > wrote:
> > > > > >> > Works with me. I will update PR to remove this.
> > > > > >> >
> > > > > >> > Also, "api_name" have been pointed out as a concern. However,
> it
> > > can
> > > > > be
> > > > > >> > handy for logging and similar purposes. Any take on that?
> > > > > >> >
> > > > > >> > On Tue, Mar 1, 2016 at 3:46 PM, Gwen Shapira <
> g...@confluent.io
> > >
> > > > > wrote:
> > > > > >> >
> > > > > >> >> Jay also mentioned:
> > > > > >> >> "Or, alternately, since deprecation has no functional impact
> > and
> > > is
> > > > > >> >> just a message
> > > > > >> >> to developers, we could just leave it out of the protocol and
> > > just
> > > > > have
> > > > > >> it
> > > > > >> >> in release notes etc."
> > > > > >> >>
> > > > > >> >> I'm in favor of leaving it out of the protocol. I can't
> really
> > > see
> > > > a
> > > > > >> >> use-case.
> > > > > >> >>
> > > > > >> >> Gwen
> > > > > >> >>
> > > > > >> >> On Mon, Feb 29, 2016 at 3:55 PM, Ashish Singh <
> > > asi...@cloudera.com
> > > > >
> > > > > >> wrote:
> > > > > >> >>
> > > > > >> >> > I hope it is OK for me to make some progress here. I have
> > made
> > > > the
> > > > > >> >> > following changes.
> > > > > >> >> >
> > > > > >> >> > 1. Updated KIP-35, to adopt Jay's suggestion on maintaining
> > > > > separate
> > > > > >> list
> > > > > >> >> > of deprecated versions, instead of using a version of -1.
> > > > > >> >> > 2. Added information on required permissions, Describe
> action
> > > on
> > > > > >> Cluster
> > > > > >> >> > resource, to be able to retrieve protocol versions from a
> > auth
> > > > > enabled
> > > > > >> >> > Kafka cluster.
> > > > > >> >> >
> > > > > >> >> > Created https://issues.apache.org/jira/browse/KAFKA-3304.
> > > > Primary
> > > > > >> patch
> > > > > >> >> is
> > > > > >> >> > available to review,
> > https://github.com/apache/kafka/pull/986
> > > > > >> >> >
> > > > > >> >> > On Thu, Feb 25, 2016 at 1:27 PM, Ashish Singh <
> > > > asi...@cloudera.com
> > > > > >
> > > > > >> >> wrote:
> > > > > >> >> >
> > > > > >> >> > > Kafka clients in Hadoop ecosystem, Flume, Spark, etc,
> have
> > > > found
> > > > > it
> > > > > >> >> > really
> > > > > >> >> > > difficult to cope up with Kafka releases as they want to
> > > > support
> > > > > >> users
> > > > > >> >> on
> > > > > >> >> > > different Kafka versions. Capability to retrieve protocol
> > > > version
> > > > > >> will
> > > > > >> >> > go a
> > > > > >> >> > > long way to ease out those pain points. I will be happy
> to
> > > help
> > > > > out
> > > > > >> >> with
> > > > > >> >> > > the work on this KIP. @Magnus, thanks for driving this,
> is
> > it
> > > > OK
> > > > > if
> > > > > >> I
> > > > > >> >> > carry
> > > > > >> >> > > forward the work from here. It will be ideal to have this
> > in
> > > > > >> 0.10.0.0.
> > > > > >> >> > >
> > > > > >> >> > > On Mon, Oct 12, 2015 at 9:29 PM, Jay Kreps <
> > j...@confluent.io
> > > >
> > > > > >> wrote:
> > > > > >> >> > >
> > > > > >> >> > >> I wonder if we need to solve the error problem? I think
> > this
> > > > KIP
> > > > > >> >> gives a
> > > > > >> >> > >> descent work around.
> > > > > >> >> > >>
> > > > > >> >> > >> Probably we should have included an error in the
> response
> > > > > header,
> > > > > >> but
> > > > > >> >> we
> > > > > >> >> > >> debated it at the time decided not to and now it is
> pretty
> > > > hard
> > > > > to
> > > > > >> add
> > > > > >> >> > >> because the headers aren't versioned (d'oh).
> > > > > >> >> > >>
> > > > > >> >> > >> It seems like any other solution is going to be kind of
> a
> > > > hack,
> > > > > >> right?
> > > > > >> >> > >> Sending malformed responses back seems like not a clean
> > > > > solution...
> > > > > >> >> > >>
> > > > > >> >> > >> (Not sure if I was pro- having a top-level error or not,
> > but
> > > > in
> > > > > any
> > > > > >> >> case
> > > > > >> >> > >> the rationale for the decision was that so many of the
> > > > requests
> > > > > >> were
> > > > > >> >> > >> per-partition or per-topic or whatever and hence fail or
> > > > > succeed at
> > > > > >> >> that
> > > > > >> >> > >> level and this makes it hard to know what the right
> > > top-level
> > > > > error
> > > > > >> >> code
> > > > > >> >> > >> is
> > > > > >> >> > >> and hard for the client to figure out what to do with
> the
> > > top
> > > > > level
> > > > > >> >> > error
> > > > > >> >> > >> if some of the partitions succeed but there is a
> top-level
> > > > > error).
> > > > > >> >> > >>
> > > > > >> >> > >> I think actually this new API actually gives a way to
> > handle
> > > > > this
> > > > > >> >> > >> gracefully on the client side by just having clients
> that
> > > want
> > > > > to
> > > > > >> be
> > > > > >> >> > >> graceful check for support for their version. Clients
> that
> > > do
> > > > > that
> > > > > >> >> will
> > > > > >> >> > >> have a graceful message.
> > > > > >> >> > >>
> > > > > >> >> > >> At some point if we're ever reworking the headers we
> > should
> > > > > really
> > > > > >> >> > >> consider
> > > > > >> >> > >> (a) versioning them and (b) adding a top-level error
> code
> > in
> > > > the
> > > > > >> >> > response.
> > > > > >> >> > >> But given this would be a big breaking change and this
> is
> > > > really
> > > > > >> just
> > > > > >> >> to
> > > > > >> >> > >> give a nicer error message seems like it probably isn't
> > > worth
> > > > > it to
> > > > > >> >> try
> > > > > >> >> > to
> > > > > >> >> > >> do something now.
> > > > > >> >> > >>
> > > > > >> >> > >> -Jay
> > > > > >> >> > >>
> > > > > >> >> > >>
> > > > > >> >> > >>
> > > > > >> >> > >> On Mon, Oct 12, 2015 at 8:11 PM, Jiangjie Qin
> > > > > >> >> <j...@linkedin.com.invalid
> > > > > >> >> > >
> > > > > >> >> > >> wrote:
> > > > > >> >> > >>
> > > > > >> >> > >> > I am thinking instead of returning an empty response,
> it
> > > > > would be
> > > > > >> >> > >> better to
> > > > > >> >> > >> > return an explicit UnsupportedVersionException code.
> > > > > >> >> > >> >
> > > > > >> >> > >> > Today KafkaApis handles the error in the following
> way:
> > > > > >> >> > >> > 1. For requests/responses using old Scala classes,
> > > KafkaApis
> > > > > uses
> > > > > >> >> > >> > RequestOrResponse.handleError() to return an error
> > > response.
> > > > > >> >> > >> > 2. For requests/response using Java classes (only
> > > > > >> JoinGroupRequest
> > > > > >> >> and
> > > > > >> >> > >> > Heartbeat now), KafkaApis calls
> > > > > >> AbstractRequest.getErrorResponse()
> > > > > >> >> to
> > > > > >> >> > >> > return an error response.
> > > > > >> >> > >> >
> > > > > >> >> > >> > In KAFKA-2512, I am returning an
> > > UnsupportedVersionException
> > > > > for
> > > > > >> >> case
> > > > > >> >> > >> [1]
> > > > > >> >> > >> > when see an unsupported version. This will put the
> error
> > > > code
> > > > > per
> > > > > >> >> > topic
> > > > > >> >> > >> or
> > > > > >> >> > >> > partition for most of the requests, but might not work
> > all
> > > > the
> > > > > >> time.
> > > > > >> >> > >> e.g.
> > > > > >> >> > >> > TopicMetadataRequest with an empty topic set.
> > > > > >> >> > >> >
> > > > > >> >> > >> > Case [2] does not quite work for unsupported version,
> > > > because
> > > > > we
> > > > > >> >> will
> > > > > >> >> > >> > thrown an uncaught exception when version is not
> > > recognized
> > > > > (BTW
> > > > > >> >> this
> > > > > >> >> > >> is a
> > > > > >> >> > >> > bug). Part of the reason is that for some response
> > types,
> > > > > error
> > > > > >> code
> > > > > >> >> > is
> > > > > >> >> > >> not
> > > > > >> >> > >> > part of the response level field.
> > > > > >> >> > >> >
> > > > > >> >> > >> > Maybe it worth checking how each response is dealing
> > with
> > > > > error
> > > > > >> code
> > > > > >> >> > >> today.
> > > > > >> >> > >> > A scan of the response formats gives the following
> > result:
> > > > > >> >> > >> > 1. TopicMetadataResponse - per topic error code, does
> > not
> > > > work
> > > > > >> when
> > > > > >> >> > the
> > > > > >> >> > >> > topic set is empty in the request.
> > > > > >> >> > >> > 2. ProduceResonse - per partition error code.
> > > > > >> >> > >> > 3. OffsetCommitResponse - per partition.
> > > > > >> >> > >> > 4. OffsetFetchResponse - per partition.
> > > > > >> >> > >> > 5. OffsetResponse - per partition.
> > > > > >> >> > >> > 6. FetchResponse - per partition
> > > > > >> >> > >> > 7. ConsumerMetadataResponse - response level
> > > > > >> >> > >> > 8. ControlledShutdownResponse - response level
> > > > > >> >> > >> > 9. JoinGroupResponse - response level
> > > > > >> >> > >> > 10. HearbeatResponse - response level
> > > > > >> >> > >> > 11. LeaderAndIsrResponse - response level
> > > > > >> >> > >> > 12. StopReplicaResponse - response level
> > > > > >> >> > >> > 13. UpdateMetadataResponse - response level
> > > > > >> >> > >> >
> > > > > >> >> > >> > So from the list above it looks for each response we
> are
> > > > > actually
> > > > > >> >> able
> > > > > >> >> > >> to
> > > > > >> >> > >> > return an error code, as long as we make sure the
> topic
> > or
> > > > > >> partition
> > > > > >> >> > >> won't
> > > > > >> >> > >> > be empty when the error code is at topic or partition
> > > level.
> > > > > >> Luckily
> > > > > >> >> > in
> > > > > >> >> > >> the
> > > > > >> >> > >> > above list we only need to worry about
> > > > TopicMetadataResponse.
> > > > > >> >> > >> >
> > > > > >> >> > >> > Maybe error handling is out of the scope of this KIP,
> > but
> > > I
> > > > > >> prefer
> > > > > >> >> we
> > > > > >> >> > >> think
> > > > > >> >> > >> > through how to deal with error code for the requests,
> > > > because
> > > > > >> there
> > > > > >> >> > are
> > > > > >> >> > >> > more request types to be added in KAFKA-2464 and
> future
> > > > > patches.
> > > > > >> >> > >> >
> > > > > >> >> > >> > Thanks,
> > > > > >> >> > >> >
> > > > > >> >> > >> > Jiangjie (Becket) Qin
> > > > > >> >> > >> >
> > > > > >> >> > >> > On Mon, Oct 12, 2015 at 6:04 PM, Jay Kreps <
> > > > j...@confluent.io>
> > > > > >> >> wrote:
> > > > > >> >> > >> >
> > > > > >> >> > >> > > Two quick pieces of feedback:
> > > > > >> >> > >> > > 1. The use of a version of -1 as magical entry
> > dividing
> > > > > between
> > > > > >> >> > >> > deprecated
> > > > > >> >> > >> > > versions is a bit hacky. What about instead having
> an
> > > > array
> > > > > of
> > > > > >> >> > >> supported
> > > > > >> >> > >> > > versions and a separate array of deprecated
> versions.
> > > The
> > > > > >> >> deprecated
> > > > > >> >> > >> > > versions would always be a subset of the supported
> > > > versions.
> > > > > >> Or,
> > > > > >> >> > >> > > alternately, since deprecation has no functional
> > impact
> > > > and
> > > > > is
> > > > > >> >> just
> > > > > >> >> > a
> > > > > >> >> > >> > > message to developers, we could just leave it out of
> > the
> > > > > >> protocol
> > > > > >> >> > and
> > > > > >> >> > >> > just
> > > > > >> >> > >> > > have it in release notes etc.
> > > > > >> >> > >> > > 2. I think including the api name may cause some
> > > problems.
> > > > > >> >> Currently
> > > > > >> >> > >> the
> > > > > >> >> > >> > > api key is the primary key that we keep consistent
> but
> > > we
> > > > > have
> > > > > >> >> > >> actually
> > > > > >> >> > >> > > evolved the english description of the apis as they
> > have
> > > > > >> changed.
> > > > > >> >> > The
> > > > > >> >> > >> > only
> > > > > >> >> > >> > > use I can think of for the name would be if people
> > used
> > > > the
> > > > > >> >> logical
> > > > > >> >> > >> name
> > > > > >> >> > >> > > and tried to resolve the api key, but that would be
> > > wrong.
> > > > > Not
> > > > > >> >> sure
> > > > > >> >> > >> if we
> > > > > >> >> > >> > > actually need the english name, if there is a use
> > case I
> > > > > guess
> > > > > >> >> we'll
> > > > > >> >> > >> just
> > > > > >> >> > >> > > have to be very clear that the name is just
> > > documentation
> > > > > and
> > > > > >> can
> > > > > >> >> > >> change
> > > > > >> >> > >> > > any time.
> > > > > >> >> > >> > >
> > > > > >> >> > >> > > -Jay
> > > > > >> >> > >> > >
> > > > > >> >> > >> > > On Fri, Sep 25, 2015 at 2:53 PM, Magnus Edenhill <
> > > > > >> >> > mag...@edenhill.se>
> > > > > >> >> > >> > > wrote:
> > > > > >> >> > >> > >
> > > > > >> >> > >> > > > Good evening,
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > > > KIP-35 was created to address current and future
> > > > > >> broker-client
> > > > > >> >> > >> > > > compatibility.
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > >
> > > > > >> >> > >> >
> > > > > >> >> > >>
> > > > > >> >> >
> > > > > >> >>
> > > > > >>
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-35+-+Retrieving+protocol+version
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > > > Summary:
> > > > > >> >> > >> > > > * allow clients to retrieve the broker's protocol
> > > > version
> > > > > >> >> > >> > > > * make broker handle unknown protocol requests
> > > > gracefully
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > > > Feedback and comments welcome!
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > > > Regards,
> > > > > >> >> > >> > > > Magnus
> > > > > >> >> > >> > > >
> > > > > >> >> > >> > >
> > > > > >> >> > >> >
> > > > > >> >> > >>
> > > > > >> >> > >
> > > > > >> >> > >
> > > > > >> >> > >
> > > > > >> >> > > --
> > > > > >> >> > >
> > > > > >> >> > > Regards,
> > > > > >> >> > > Ashish
> > > > > >> >> > >
> > > > > >> >> >
> > > > > >> >> >
> > > > > >> >> >
> > > > > >> >> > --
> > > > > >> >> >
> > > > > >> >> > Regards,
> > > > > >> >> > Ashish
> > > > > >> >> >
> > > > > >> >>
> > > > > >> >
> > > > > >> >
> > > > > >> >
> > > > > >> > --
> > > > > >> >
> > > > > >> > Regards,
> > > > > >> > Ashish
> > > > > >>
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > >
> > > > > > Regards,
> > > > > > Ashish
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > >
> > > > Regards,
> > > > Ashish
> > > >
> > >
> >
>
