In kafka-python we've been doing something like:

if version >= (0, 9):
  Do cool new stuff
elif version >= (0, 8, 2):
  Do some older stuff
....
else:
  raise UnsupportedVersionError

This will break if / when the new 0.9 apis are completely removed from some
future release, but should handle intermediate broker upgrades. Because we
can't add support for future apis a priori, I think the best we could do
here is throw an error that request protocol version X is not supported.
For now that comes through as a broken socket connection, so there is an
error - just not a super helpful one.

For that reason I'm also in favor of a generic error response when a
protocol req is not recognized.

-Dana
On Mar 2, 2016 5:38 PM, "Jay Kreps" <j...@confluent.io> wrote:

> 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
> > > > >
> > > >
> > >
> >
>

Reply via email to