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 >
