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
