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 >
