I am ok with #3 as well. On Mon, May 9, 2016 at 12:12 PM, Ismael Juma <ism...@juma.me.uk> wrote:
> +1 for option 3 (unfortunately all options have pros and cons, but I think > option 3 does a better job with regards to trade-offs). > > Ismael > > On Mon, May 9, 2016 at 7:56 PM, Gwen Shapira <g...@confluent.io> wrote: > > > We need to conclude this discussion in order to unblock the release. > > > > If I understand correctly, we have 3 options on the table: > > > > 1. Add the missing APIs as "deprecated". > > Pros: Conceptually, the right way to get rid of an API without breaking > > Storm > > Cons: Users of the new API will get ugly warnings, and they are > > actually doing the right thing. This will encourage bad usage of our > > APIs. > > > > 2. Add the missing APIs and sticking with them forever. > > Pros: Avoid breaking Storm and avoid ugly warnings > > Cons: Uglier and more confusing APIs for Consumer for eternity. We > > marked the API as @unstable specifically to avoid this scenario. > > > > 3. Leave things as is. Storm (and maybe few others) will need to do a > > bit of reflection to support the new consumer in both 0.9.0 and 0.10.0 > > branches. > > Pros: Clean APIs for our Consumer, clear meaning for "@unstable" in > > the community. > > Cons: Some extra work for Storm. > > > > With Apache Kafka's interests in mind, I think #3 is the right way > > forward. Anything else is worse for Kafka users. > > > > I'd love to hear from the rest of the members :) > > > > Gwen > > > > On Sun, May 8, 2016 at 1:56 AM, Ismael Juma <ism...@juma.me.uk> wrote: > > > Hi Harsha, > > > > > > See inline. > > > > > > On Sun, May 8, 2016 at 3:02 AM, Harsha <ka...@harsha.io> wrote: > > > > > >> Ismael, > > >> Do we need to add old assign and subscribe that accepts List. > > > > > > > > > Yes, to maintain binary compatibility. If you compile your jar with > > > kafka-clients 0.9.0.1 and then run it with kafka-clients 0.10.0.0 you > > will > > > get an error like: > > > > > > ~/t/binary-compat-test ❯❯❯ java -cp > > > > > > src:lib/kafka-clients-0.10.0.0.jar:lib/slf4j-api-1.7.21.jar:lib/slf4j-log4j12-1.7.21.jar:lib/log4j-1.2.17.jar > > > test.BinaryCompat > > > Exception in thread "main" java.lang.NoSuchMethodError: > > > > > > org.apache.kafka.clients.consumer.KafkaConsumer.subscribe(Ljava/util/List;)V > > > at test.BinaryCompat.main(BinaryCompat.java:21) > > > > > > That is a real error from a simple test I did. This is why I asked in > my > > > original message if you had tested that your proposed PR fixed your > issue > > > completely (I don't think it does). > > > > > > It will get implicitly cast to collection with the new methods. > > >> > > > > > > That is only the case if the code is recompiled with 0.10.0.0 (i.e. > > source > > > compatibility). > > > > > > Hope this makes things clearer. > > > > > > Ismael > > > > > > P.S. Source code for the simple test I did: > > > https://gist.github.com/ijuma/dfec36382779b5022989b4380af99b37 > > > > > > > > >> The only problem comes from the methods that accepts varargs. > > >> > > >> -Harsha > > >> > > >> On Sat, May 7, 2016, at 05:53 PM, Mark Grover wrote: > > >> > Thanks Ismael, I agree with you, it makes sense to leave things the > > way > > >> > they are in Kafka 0.10. > > >> > > > >> > On Fri, May 6, 2016 at 5:27 PM, Ismael Juma <ism...@juma.me.uk> > > wrote: > > >> > > > >> > > Hi Mark, > > >> > > > > >> > > Thanks for the email. First of all, I'd like to mention that the > > >> `Unstable` > > >> > > annotation has been removed from the new Java consumer in 0.10, so > > you > > >> can > > >> > > expect compatibility from now on. We definitely understand that > > >> > > compatibility is important for widespread adoption. > > >> > > > > >> > > The current PR for KAFKA-3633 adds deprecated and overloaded > methods > > >> for > > >> > > `seekToBeginning`, `seekToEnd`, `pause` and `resume` each taking a > > >> varargs > > >> > > parameter for backwards compatibility. If these methods solved the > > >> binary > > >> > > compatibility issue, I'd be supportive of adding them. > > >> > > > > >> > > However, as I pointed out in my original message (and Jason > > elaborated > > >> > > subsequently), something would also have to be done about `assign` > > and > > >> > > `subscribe` in order to maintain binary compatibility between 0.9 > > and > > >> 0.10. > > >> > > And a good solution for these methods is elusive. > > >> > > > > >> > > If we add deprecated and overloaded methods that take a `List` > > >> parameter, > > >> > > then every existing user of the new consumer will be exposed to a > > >> > > deprecated warning (or error if they have a warning as error build > > >> policy) > > >> > > because everyone uses `subscribe`. Avoiding the warning involves > > using > > >> > > `Set` instead of `List`, which is a bit weird and unintuitive > > >> (although we > > >> > > could document it). > > >> > > > > >> > > We could add the overloaded methods without deprecating them. In > > this > > >> case, > > >> > > we would be stuck with two methods for the same thing forever (for > > both > > >> > > `subscribe` and `assign`). This makes the API more confusing and > > >> overloads > > >> > > mean that type inference from lambdas would be less effective (if > at > > >> all > > >> > > effective). > > >> > > > > >> > > Or we could leave things as they are. The `subscribe` and `assign` > > >> changes > > >> > > are source compatible so no source changes are needed by the > common > > >> user > > >> > > who just compiles against a particular version of the Kafka > clients > > >> > > library. It's also important to note that kafka-clients 0.9 works > > fine > > >> with > > >> > > 0.10 brokers. Supporting both 0.9 and 0.10 clients from the same > JAR > > >> will > > >> > > be a bit annoying, but the ugly shim code for that is > > straightforward > > >> to > > >> > > write for advanced users that need this. > > >> > > > > >> > > I should make it clear that this is my position, other committers > > may > > >> feel > > >> > > differently. > > >> > > > > >> > > Ismael > > >> > > > > >> > > On Sat, May 7, 2016 at 12:38 AM, Mark Grover <m...@apache.org> > > wrote: > > >> > > > > >> > > > 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 > > >> > > > > > > >> > > > > > >> > > > > >> > > > > >
