I'm assuming that the broker would continue to support older versions of the requests, so the check would probably be more like this:
if (version > 0.9) // send 0.9 requests else if (version > 0.8) // send 0.8 requests Does that not work? This version can also can be used by clients to tell whether the broker supports major new features (such as the group coordinator protocol or the admin APIs). -Jason On Wed, Mar 2, 2016 at 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 > > > > > > > > > > > > > > >
