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