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
