Hi Steven, I noticed you are making KafkaFuture.addWaiter(...) public as part of your PR. This is a very useful method to add – and you should mention it in the KIP – however addWaiter currently doesn't guard against exceptions thrown inside of the BiConsumer function, which is something we should probably fix before making it public.
I was about to make the necessary exception handling changes as part of https://github.com/apache/kafka/pull/4308 until someone pointed out your KIP to me. Since you already have a PR out, it might be worth incorporating my fixes (and the extra docs), what do you think? I'll rebase my PR onto yours to make it easier to merge. Thanks! Xavier On Mon, Dec 4, 2017 at 4:03 AM Steven Aerts <steven.ae...@gmail.com> wrote: > Tom, > > Thanks for the review. > updated the motivation a little bit, it's better, but I have to admit can > be improved. > I made addWaiters public. > > Enjoy, > > Steven > > > > Op ma 4 dec. 2017 om 11:01 schreef Tom Bentley <t.j.bent...@gmail.com>: > > > Hi Steven, > > > > Thanks for updating the KIP. I have a couple of points: > > > > 1. Typo in the first sentence of the Motivation. Also what does "empty > > public abstract classes with one abstract method" mean -- if it's got one > > abstract method in what way is it empty? > > 2.From an entirely self-centred point of view, the main thing that's > > missing for my work in KIP-183 is that addWaiter() needs to be public. > > > > Thanks again, > > > > Tom > > > > On 2 December 2017 at 10:07, Steven Aerts <steven.ae...@gmail.com> > wrote: > > > > > Hi Tom, > > > > > > I just made changes to the proposal of KIP-218, to make everything more > > > backwards compatible as suggested by Collin. > > > For me it is now in a state where starts to become final. > > > > > > I propose to wait a few days so everybody can take a look and open the > > > votes when I do not receive any major comments. > > > > > > Does that sound ok for you? > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > 218%3A+Make+KafkaFuture.Function+java+8+lambda+compatible > > > > > > Thanks for your patience, > > > > > > > > > Steven > > > > > > > > > Op vr 1 dec. 2017 om 11:55 schreef Tom Bentley <t.j.bent...@gmail.com > >: > > > > > > > Hi Steven, > > > > > > > > I'm particularly interested in seeing progress on this KIP as the > work > > > for > > > > KIP-183 needs a public version of BiConsumer. do you have any idea > when > > > the > > > > KIP might be ready for voting? > > > > > > > > Thanks, > > > > > > > > Tom > > > > > > > > On 10 November 2017 at 13:38, Steven Aerts <steven.ae...@gmail.com> > > > wrote: > > > > > > > > > Collin, Ben, > > > > > > > > > > Thanks for the input. > > > > > > > > > > I will work out this proposa, so I get an idea on the impact. > > > > > > > > > > Do you think it is a good idea to line up the new method names with > > > those > > > > > of CompletableFuture? > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > Steven > > > > > > > > > > Op vr 10 nov. 2017 om 12:12 schreef Ben Stopford <b...@confluent.io > >: > > > > > > > > > > > Sounds like a good middle ground to me. What do you think Steven? > > > > > > > > > > > > On Mon, Nov 6, 2017 at 8:18 PM Colin McCabe <cmcc...@apache.org> > > > > wrote: > > > > > > > > > > > > > It would definitely be nice to use the jdk8 > CompletableFuture. I > > > > think > > > > > > > that's a bit of a separate discussion, though, since it has > such > > > > heavy > > > > > > > compatibility implications. > > > > > > > > > > > > > > How about making KIP-218 backwards compatible? As a starting > > > point, > > > > > you > > > > > > > can change KafkaFuture#BiConsumer to an interface with no > > > > compatibility > > > > > > > implications, since there are currently no public functions > > exposed > > > > > that > > > > > > > use it. That leaves KafkaFuture#Function, which is publicly > used > > > > now. > > > > > > > > > > > > > > For the purposes of KIP-218, how about adding a new interface > > > > > > > FunctionInterface? Then you can add a function like this: > > > > > > > > > > > > > > > public abstract <R> KafkaFuture<R> > > > thenApply(FunctionInterface<T, > > > > R> > > > > > > > function); > > > > > > > > > > > > > > And mark the older declaration as deprecated: > > > > > > > > > > > > > > > @deprecated > > > > > > > > public abstract <R> KafkaFuture<R> thenApply(Function<T, R> > > > > > function); > > > > > > > > > > > > > > This is a 100% compatible way to make things nicer for java 8. > > > > > > > > > > > > > > cheers, > > > > > > > Colin > > > > > > > > > > > > > > > > > > > > > On Thu, Nov 2, 2017, at 10:38, Steven Aerts wrote: > > > > > > > > Hi Tom, > > > > > > > > > > > > > > > > Nice observation. > > > > > > > > I changed "Rejected Alternatives" section to "Other > > > Alternatives", > > > > as > > > > > > > > I see myself as too much of an outsider to the kafka > community > > to > > > > be > > > > > > > > able to decide without this discussion. > > > > > > > > > > > > > > > > I see two major factors to decide: > > > > > > > > - how soon will KIP-118 (drop support of java 7) be > > implemented? > > > > > > > > - for which reasons do we drop backwards compatibility for > > > public > > > > > > > > interfaces marked as Evolving > > > > > > > > > > > > > > > > If KIP-118 which is scheduled for version 2.0.0 is going to > be > > > > > > > > implemented soon, I agree with you that replacing KafkaFuture > > > with > > > > > > > > CompletableFuture (or CompletionStage) is a preferable > option. > > > > > > > > But as I am not familiar with the roadmap it is difficult to > > tell > > > > for > > > > > > me. > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > > > > > Steven > > > > > > > > > > > > > > > > > > > > > > > > 2017-11-02 11:27 GMT+01:00 Tom Bentley < > t.j.bent...@gmail.com > > >: > > > > > > > > > Hi Steven, > > > > > > > > > > > > > > > > > > I notice you've renamed the template's "Rejected > > Alternatives" > > > > > > section > > > > > > > to > > > > > > > > > "Other Alternatives", suggesting they're not rejected yet > > (or, > > > if > > > > > you > > > > > > > have > > > > > > > > > rejected them, I think you should give your reasons). > > > > > > > > > > > > > > > > > > Personally, I'd like to understand the arguments against > > simply > > > > > > > replacing > > > > > > > > > KafkaFuture with CompletableFuture in Kafka 2.0. In other > > > words, > > > > if > > > > > > we > > > > > > > were > > > > > > > > > starting without needing to support Java 7 what would be > the > > > > > > arguments > > > > > > > for > > > > > > > > > having our own KafkaFuture? > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > Tom > > > > > > > > > > > > > > > > > > On 1 November 2017 at 16:01, Ted Yu <yuzhih...@gmail.com> > > > wrote: > > > > > > > > > > > > > > > > > >> KAFKA-4423 is still open. > > > > > > > > >> When would Java 7 be dropped ? > > > > > > > > >> > > > > > > > > >> Thanks > > > > > > > > >> > > > > > > > > >> On Wed, Nov 1, 2017 at 8:56 AM, Ismael Juma < > > > ism...@juma.me.uk> > > > > > > > wrote: > > > > > > > > >> > > > > > > > > >> > On Wed, Nov 1, 2017 at 3:51 PM, Ted Yu < > > yuzhih...@gmail.com > > > > > > > > > > wrote: > > > > > > > > >> > > > > > > > > > >> > > bq. Wait for a kafka release which will not support > > java 7 > > > > > > anymore > > > > > > > > >> > > > > > > > > > > >> > > Do you want to raise a separate thread for the above ? > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > There is already a KIP for this so a separate thread is > > not > > > > > > needed. > > > > > > > > >> > > > > > > > > > >> > Ismael > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > >
