I understand and empathize with both sides of the story here. I spend some of my time on Spark and Kafka integration and I have cc'ed Cody who's been working on new Kafka consumer API with Spark Streaming. Spark hasn't merged the new Kafka consumer API integration, the PR is up and we, as a community, are deliberating <https://issues.apache.org/jira/browse/SPARK-12177?focusedCommentId=15274910&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15274910> if now is the right time to put this in, given the flux in the API, the lack of delegation tokens support, etc.
The proposed Spark integration with Kafka's new API relies on KafkaConsumer::pause() and KafkaConsumer::seekToEnd() and those methods break compatibility between 0.9 and 0.10 RC4 (since KAFKA-3633 <https://issues.apache.org/jira/browse/KAFKA-3633> remains unresolved). What this means is that if Spark supports both 0.9 and 0.10, we are going to have some complexity to compile against 0.9 and 0.10. So, Spark community, Cody and myself are all leaning towards not putting Kafka 0.9 support in, and only supporting starting Kafka 0.10 (or, may be even later) depending on the compatibility situation. The point I am trying to make is that the new consumer API doesn't add much value for us just yet and breaking compatibility doesn't help in encouraging us to add support for it in Spark. As far as this particular topic (KAFKA-3633) goes, I don't have a strong vote, since Spark isn't likely to support 0.9's new kafka consumer API anyways. However, I'd state the obvious that compatibility is important if you'd like to encourage us to adopt the new API. Mark On Mon, May 2, 2016 at 10:45 AM, Guozhang Wang <wangg...@gmail.com> wrote: > Just my two cents here: > > I agree with Ewen and Grant on the indication of the "unstable" annotations > of being possible for backward incompatible. That means, users can make a > call themselves of whether to start trying out the new APIs / libraries > with the risk or changing code when it changes in a later release or just > wait for it to be stable. Personally I don't think it would result in no > one ever going to try out "unstable" new APIs, even in their production > usages (for example at LI we used the LiKafkaClient to wrap the apache > kafka clients and one motivation is to abstract any API backward > incompatibility); for cases like the Storm integration scenarios, yes it > may become a considering blocker for incorporating "unstable" APIs, again I > think it is just a decision we will educate users to make themselves. > > > Guozhang > > > On Sat, Apr 30, 2016 at 8:13 AM, Harsha <ka...@harsha.io> wrote: > > > Hi Jason, > > Yes I am in favor removing them 0.11 and it definitely gives > > everyone one major version to update their clients to remove > > the deprecated commands. > > > > Thanks, > > Harsha > > > > On Fri, Apr 29, 2016, at 11:02 PM, Ewen Cheslack-Postava wrote: > > > I agree with Grant that we really need to indicate to consumers of APIs > > > that when we mark it as unstable, it *really* means unstable. This is a > > > more general problem of needing to define our APIs and stability -- but > > > I's > > > say that while we probably were too hasty in adding APIs, it was > probably > > > better to add *some* indication of stability and support than just add > > > APIs > > > with not promises. > > > > > > On the other hand, since I helped introduce the Unstable annotation, > even > > > then it wasn't entirely clear what it meant, and I am a firm believer > in > > > attempting to provide *some* migration period for incompatible > changes, I > > > would be more than happy to adapt the public API to provide backwards > > > compatibility for those APIs for *at least* one release. > > > > > > Is there a strong reason for not doing this that isn't incompatible? > > > > > > And shouldn't we try to be as helpful to consumers of our *new* APIs as > > > possible -- we want them to adopt new APIs! If there's a small amount > of > > > effort on our part that keeps things compatible, at least over the > course > > > of a major release, it encourages downstream projects to try our APIs > > > earlier, and that's a good thing. It won't always be perfect; sometimes > > > we'll need to break major new features in a minor release; but in > > > general, > > > won't it be better? > > > > > > We should be very clear that we are going to remove these APIs with the > > > 0.11 release, which should hopefully make it clear what Storm can > expect > > > from us in terms of compatibility (noting, of course, that we make no > > > real > > > promises currently about how long 0.10.x releases will be made! we > > > already > > > make few guarantees about long term support). > > > > > > I know it would be ideal if all "external" stakeholders could get their > > > vote in with the KIP, but it's probably unrealistic to expect that to > > > happen any time soon -- not everyone will see developments in the Kafka > > > project. I think we should give *a bit* of flexibility, especially for > > > stuff we were all on the fence about, when these types of issues come > up. > > > > > > Everyone seemed to be on the fence previously. Is there a good reason > not > > > to adopt the suggested changes, that cost of a bit of compatibility > pain? > > > > > > -Ewen > > > > > > > > > On Fri, Apr 29, 2016 at 7:58 PM, Jason Gustafson <ja...@confluent.io> > > > wrote: > > > > > > > Hey Harsha, > > > > > > > > Just to clarify, are you ok with removing the methods in a later > > release > > > > (say 0.11)? As I mentioned above, the only weird ones are subscribe() > > and > > > > assign(), which will have a deprecated version which accepts List. > > Users > > > > will have to change their code to use another collection type or a > > typecast > > > > to avoid deprecation warnings. That's annoying, but maybe better than > > > > breaking compatibility. Does it make sense to update the KIP with > your > > > > proposal and request a new vote? > > > > > > > > Thanks, > > > > Jason > > > > > > > > On Fri, Apr 29, 2016 at 4:25 PM, Harsha <ka...@harsha.io> wrote: > > > > > > > > > Grant, > > > > > I am sure this is discussed and voted. I've seen the > > > > > discussion. Given that there is an opportunity to make it > > less > > > > > painful for the users who shipped consumers using the > 0.9.x > > we > > > > > should consider that. > > > > > ". However, for now the documentation of > > > > > > > the Unstable annotation says, "No guarantee is provided as to > > > > > reliability > > > > > > > or stability across any level of release granularity." If we > > can't > > > > > > > leverage the Unstable annotation to make breaking changes where > > > > > necessary, > > > > > > > it will be tough to vet new apis without generating a lot of > > > > deprecated > > > > > > > code." > > > > > Yes we can tell everyone thats because we marked api unstable we > > gonna > > > > > break it in future release and not even consider make it > compatible. > > > > > With this approach I am sure no one would be interested in writing > or > > > > > using any of the api's until they are stable and thats not way to > vet > > > > > new apis. > > > > > > > > > > -Harsha > > > > > > > > > > > > > > > > > > > > On Fri, Apr 29, 2016, at 10:39 AM, Grant Henke wrote: > > > > > > If anyone wants to review the KIP call discussion we had on this > > just > > > > > > before the vote, here is a link to the relevant session (6 > minutes > > in): > > > > > > https://youtu.be/Hcjur17TjBE?t=6m > > > > > > > > > > > > On Fri, Apr 29, 2016 at 12:21 PM, Grant Henke < > ghe...@cloudera.com > > > > > > > > > wrote: > > > > > > > > > > > > > I think you are right Jason. People were definitely on the > fence > > > > about > > > > > > > this and we went back and forth for quite some time. > > > > > > > > > > > > > > I think the main point in the KIP discussion that made this > > decision, > > > > > is > > > > > > > that the Consumer was annotated with the Unstable annotation. > > Given > > > > how > > > > > > > new the Consumer is, we wanted to leverage that to make sure > the > > > > > interface > > > > > > > is clean. The same will be true for KafkaStreams in the > upcoming > > > > > release. > > > > > > > > > > > > > > We did agree that we should discuss the annotations and what > our > > > > > > > compatibility story is in the future. However, for now the > > > > > documentation of > > > > > > > the Unstable annotation says, "No guarantee is provided as to > > > > > reliability > > > > > > > or stability across any level of release granularity." If we > > can't > > > > > > > leverage the Unstable annotation to make breaking changes where > > > > > necessary, > > > > > > > it will be tough to vet new apis without generating a lot of > > > > deprecated > > > > > > > code. > > > > > > > > > > > > > > Note: We did remove the Unstable annotation from the Consumer > > > > interface > > > > > > > for 0.10 implying that it is now stable. (KAFKA-3435 > > > > > > > <https://issues.apache.org/jira/browse/KAFKA-3435>) > > > > > > > > > > > > > > Thanks, > > > > > > > Grant > > > > > > > > > > > > > > On Fri, Apr 29, 2016 at 12:05 PM, Jason Gustafson < > > > > ja...@confluent.io> > > > > > > > wrote: > > > > > > > > > > > > > >> Hey Harsha, > > > > > > >> > > > > > > >> One issue with adding back subscribe(List), but marking it > > > > deprecated > > > > > is > > > > > > >> that it may confuse some users if they use the typical > > > > Arrays.asList() > > > > > > >> pattern. You'd have to cast to a Collection to avoid the > > deprecation > > > > > > >> warning, which is awkward. Maybe it would be better in that > > case to > > > > > keep > > > > > > >> the List alternatives forever? > > > > > > >> > > > > > > >> In general, I'm not opposed to adding the methods back. When > we > > > > voted > > > > > on > > > > > > >> KIP-45, I think many of us were on the fence anyway. It would > be > > > > nice > > > > > to > > > > > > >> hear what others think. > > > > > > >> > > > > > > >> -Jason > > > > > > >> > > > > > > >> On Thu, Apr 28, 2016 at 6:30 PM, Harsha <ka...@harsha.io> > > wrote: > > > > > > >> > > > > > > >> > Hi Jason, > > > > > > >> > "t. I think what you're > > > > > > >> > saying is that the KafkaSpout has been compiled against the > > 0.9 > > > > > client, > > > > > > >> > but > > > > > > >> > it may need to be to run against 0.10 (if the user depends > on > > that > > > > > > >> > version > > > > > > >> > instead). Is that correct?" > > > > > > >> > > > > > > > >> > Yes thats true. > > > > > > >> > > > > > > > >> > " Is that correct? In general, are you expecting that > > KafkaSpout > > > > > > >> > > > will work with any kafka-clients greater than 0.9?" > > > > > > >> > > > > > > > >> > In general yes . But given that interface is marked unstable > > its > > > > > > >> > probably not reasonable to expect to work across the new > > versions > > > > > > >> > of the Kafka. > > > > > > >> > > > > > > > >> > "Another question that > > > > > > >> > > > comes to mind is whether we would also need to revert to > > the > > > > old > > > > > > >> > versions > > > > > > >> > > > of subscribe() and assign(). > > > > > > >> > Yes you are right on these methods. We need to add for these > > two > > > > as > > > > > > >> > well. > > > > > > >> > > > > > > > >> > > > > > > > >> > My issue is users who built their clients using 0.9.x java > api > > > > will > > > > > have > > > > > > >> > to change once the 0.10 release is out. Alternative I am > > proposing > > > > > is to > > > > > > >> > give these users time to move onto the new api thats added > and > > > > keep > > > > > the > > > > > > >> > old methods with deprecated tag for atleast one version. > > > > > > >> > > > > > > > >> > Thanks, > > > > > > >> > Harsha > > > > > > >> > > > > > > > >> > > > > > > > >> > On Thu, Apr 28, 2016, at 04:41 PM, Grant Henke wrote: > > > > > > >> > > FYI. I have attached a sample clients API > > change/compatibility > > > > > report > > > > > > >> to > > > > > > >> > > KAFKA-1880 < > > https://issues.apache.org/jira/browse/KAFKA-1880>. > > > > > The > > > > > > >> > report > > > > > > >> > > shows changes in the public apis between the 0.9 and trunk > > > > > branches. > > > > > > >> Some > > > > > > >> > > of them are expected per KIP-45 obviously. > > > > > > >> > > > > > > > > >> > > Thanks, > > > > > > >> > > Grant > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > On Thu, Apr 28, 2016 at 6:33 PM, Jason Gustafson < > > > > > ja...@confluent.io> > > > > > > >> > > wrote: > > > > > > >> > > > > > > > > >> > > > Hey Harsha, > > > > > > >> > > > > > > > > > >> > > > We're just trying to understand the problem first. I > think > > > > what > > > > > > >> you're > > > > > > >> > > > saying is that the KafkaSpout has been compiled against > > the > > > > 0.9 > > > > > > >> > client, but > > > > > > >> > > > it may need to be to run against 0.10 (if the user > > depends on > > > > > that > > > > > > >> > version > > > > > > >> > > > instead). Is that correct? In general, are you expecting > > that > > > > > > >> > KafkaSpout > > > > > > >> > > > will work with any kafka-clients greater than 0.9? > Another > > > > > question > > > > > > >> > that > > > > > > >> > > > comes to mind is whether we would also need to revert to > > the > > > > old > > > > > > >> > versions > > > > > > >> > > > of subscribe() and assign(). The argument type was > changed > > > > from > > > > > > >> List to > > > > > > >> > > > Collection, which is not binary compatible, right? > > > > > > >> > > > > > > > > > >> > > > Thanks, > > > > > > >> > > > Jason > > > > > > >> > > > > > > > > > >> > > > On Thu, Apr 28, 2016 at 1:41 PM, Harsha < > ka...@harsha.io> > > > > > wrote: > > > > > > >> > > > > > > > > > >> > > > > Hi Ismael, > > > > > > >> > > > > This will solve both binary and source > > > > > > >> compatibility. > > > > > > >> > > > > Storm has new KafkaSpout that used 0.9.x > > new > > > > > > >> KafkaSpout > > > > > > >> > > > > API. As part of that spout we used > > > > > > >> > > > > KafkaConsumer.seekToBeginning and other > > > > methods. > > > > > > >> Since > > > > > > >> > the > > > > > > >> > > > > method signature changed as part of > > KIP-45. If > > > > > we > > > > > > >> > update > > > > > > >> > > > > the version to 0.10 we are breaking the > > > > > > >> KafkaConsumer > > > > > > >> > > > > calls in our Storm spout. In storm's > case > > we > > > > ask > > > > > > >> users > > > > > > >> > to > > > > > > >> > > > > create uber jar with all the required > > > > > dependencies > > > > > > >> and > > > > > > >> > > > > users can free to use which version of > > kafka > > > > > they > > > > > > >> can > > > > > > >> > to > > > > > > >> > > > > be part of uber jar. If they use storm > 1.0 > > > > > release > > > > > > >> > version > > > > > > >> > > > > of storm-kafka with kafka 0.10 than it > > will > > > > > create > > > > > > >> > issues > > > > > > >> > > > > without the patch. > > > > > > >> > > > > I am still not getting clear answer here. > > Whats > > > > > > >> exactly > > > > > > >> > the > > > > > > >> > > > > issue in having these methods with > > deprecated > > > > > tag? we > > > > > > >> > keep > > > > > > >> > > > > the interface as it is. > > > > > > >> > > > > > > > > > > >> > > > > Thanks, > > > > > > >> > > > > Harsha > > > > > > >> > > > > > > > > > > >> > > > > On Thu, Apr 28, 2016, at 01:27 PM, Ismael Juma wrote: > > > > > > >> > > > > > Hi Harsha, > > > > > > >> > > > > > > > > > > > >> > > > > > What is the aim of the PR, is it to fix binary > > > > > compatibility, > > > > > > >> > source > > > > > > >> > > > > > compatibility or both? I think it only fixes source > > > > > > >> compatibility, > > > > > > >> > so I > > > > > > >> > > > > > am > > > > > > >> > > > > > interested in what testing has been done to ensure > > that > > > > > this fix > > > > > > >> > solves > > > > > > >> > > > > > the > > > > > > >> > > > > > Storm issue. > > > > > > >> > > > > > > > > > > > >> > > > > > Thanks, > > > > > > >> > > > > > Ismael > > > > > > >> > > > > > > > > > > > >> > > > > > On Thu, Apr 28, 2016 at 12:58 PM, Harsha < > > ka...@harsha.io > > > > > > > > > > > >> wrote: > > > > > > >> > > > > > > > > > > > >> > > > > > > Hi, > > > > > > >> > > > > > > We missed this vote earlier and realized > > thats > > > > its > > > > > > >> > breaking > > > > > > >> > > > the > > > > > > >> > > > > > > 0.9.x client api compatibility. I opened a > > JIRA > > > > > here > > > > > > >> > > > > > > > > https://issues.apache.org/jira/browse/KAFKA-3633 > > > > . > > > > > > >> Can we > > > > > > >> > > > keep > > > > > > >> > > > > > > the old methods with deprecated tag in 0.10 > > > > > release. > > > > > > >> > > > > > > > > > > > > >> > > > > > > Thanks, > > > > > > >> > > > > > > Harsha > > > > > > >> > > > > > > > > > > > > >> > > > > > > On Fri, Mar 18, 2016, at 01:51 PM, Jason Gustafson > > > > wrote: > > > > > > >> > > > > > > > Looks like the KIP has passed. The finally tally > > is +5 > > > > > among > > > > > > >> > > > > committers > > > > > > >> > > > > > > > and > > > > > > >> > > > > > > > +9 overall. > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > Thanks to Pierre-Yves Ritschard for all of the > > hard > > > > > work and > > > > > > >> > > > > persistence > > > > > > >> > > > > > > > with this! > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > -Jason > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > On Wed, Mar 16, 2016 at 9:01 PM, Ewen > > Cheslack-Postava > > > > > > >> > > > > > > > <e...@confluent.io> > > > > > > >> > > > > > > > wrote: > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > +1. > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > Normally I'd be more of a stickler for > > > > compatibility, > > > > > but > > > > > > >> > this is > > > > > > >> > > > > new, > > > > > > >> > > > > > > I > > > > > > >> > > > > > > > > think it's worth emphasizing that unstable > > actually > > > > > means > > > > > > >> > > > unstable > > > > > > >> > > > > & > > > > > > >> > > > > > > might > > > > > > >> > > > > > > > > require recompile (and maybe even adapting > code > > when > > > > > we > > > > > > >> > think the > > > > > > >> > > > > > > change > > > > > > >> > > > > > > > > warrants it), and I think the impact is > > relatively > > > > low > > > > > > >> since > > > > > > >> > > > those > > > > > > >> > > > > > > adopting > > > > > > >> > > > > > > > > the new consumer know that it's very new. > Agreed > > > > with > > > > > > >> > Guozhang > > > > > > >> > > > that > > > > > > >> > > > > > > better > > > > > > >> > > > > > > > > documenting the annotations will help (and > > > > personally > > > > > > >> > apologize > > > > > > >> > > > for > > > > > > >> > > > > > > that > > > > > > >> > > > > > > > > since we hastily introduced the annotations to > > give > > > > > > >> ourselves > > > > > > >> > > > > wiggle > > > > > > >> > > > > > > room > > > > > > >> > > > > > > > > on Connect). > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > -Ewen > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > On Wed, Mar 16, 2016 at 5:17 PM, Joel Koshy < > > > > > > >> > jjkosh...@gmail.com > > > > > > >> > > > > > > > > > > >> > > > > > > wrote: > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > +1 > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > On Tue, Mar 15, 2016 at 2:18 PM, Jason > > Gustafson < > > > > > > >> > > > > ja...@confluent.io > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > wrote: > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > I'd like to open the vote for KIP-45. > We've > > > > > discussed > > > > > > >> > several > > > > > > >> > > > > > > > > > alternatives > > > > > > >> > > > > > > > > > > on the mailing list and in the KIP call, > but > > > > this > > > > > > >> vote is > > > > > > >> > > > only > > > > > > >> > > > > on > > > > > > >> > > > > > > the > > > > > > >> > > > > > > > > > > documented KIP: > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61337336 > > > > > > >> . > > > > > > >> > > > > > > > > > > This > > > > > > >> > > > > > > > > > > change will not be compatible with 0.9, > but > > it > > > > > will > > > > > > >> > provide a > > > > > > >> > > > > > > cleaner > > > > > > >> > > > > > > > > API > > > > > > >> > > > > > > > > > > long term for users to work with. This is > > really > > > > > the > > > > > > >> last > > > > > > >> > > > > chance to > > > > > > >> > > > > > > > > make > > > > > > >> > > > > > > > > > an > > > > > > >> > > > > > > > > > > incompatible change like this with 0.10 > > shortly > > > > > on the > > > > > > >> > way, > > > > > > >> > > > but > > > > > > >> > > > > > > > > > compatible > > > > > > >> > > > > > > > > > > options (such as method overloading) could > > be > > > > > brought > > > > > > >> up > > > > > > >> > > > again > > > > > > >> > > > > in > > > > > > >> > > > > > > the > > > > > > >> > > > > > > > > > > future if we find it's needed. > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > > > > Thanks, > > > > > > >> > > > > > > > > > > Jason > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > -- > > > > > > >> > > > > > > > > Thanks, > > > > > > >> > > > > > > > > Ewen > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > -- > > > > > > >> > > Grant Henke > > > > > > >> > > Software Engineer | Cloudera > > > > > > >> > > gr...@cloudera.com | twitter.com/gchenke | > > > > > linkedin.com/in/granthenke > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > Grant Henke > > > > > > > Software Engineer | Cloudera > > > > > > > gr...@cloudera.com | twitter.com/gchenke | > > > > linkedin.com/in/granthenke > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Grant Henke > > > > > > Software Engineer | Cloudera > > > > > > gr...@cloudera.com | twitter.com/gchenke | > > linkedin.com/in/granthenke > > > > > > > > > > > > > > > > > > > > > -- > > > Thanks, > > > Ewen > > > > > > -- > -- Guozhang >
