Thanks for the KIP, Steven. A few minor comments: 1. The KIP seems to rely on the pull request for some of the details of the proposal. Generally, the KIP should stand on its own. For example, the public interfaces section should include the signature of interfaces and methods being added.
2. Do we really need to deprecate `Function`? This will add build noise to any library that builds with 1.1+ but also wants to support 0.11 and 1.0. It may be worth just documenting that `FunctionInterface` is the one to use for lambda support. 3. `FunctionInterface` is a bit of a clunky name. Due to lambdas, users don't have to type the name themselves, so maybe it's fine as it is. An alternative would be `BaseFunction` or something like that. Ismael On Tue, Dec 12, 2017 at 4:24 PM, Xavier Léauté <xav...@confluent.io> wrote: > I'm fine with the whenComplete solution as well. > On Tue, Dec 12, 2017 at 03:57 Tom Bentley <t.j.bent...@gmail.com> wrote: > > > Hi Steven, > > > > I am happy with adding whenComplete() instead of addWaiter(), > > > > Cheers, > > > > Tom > > > > On 12 December 2017 at 11:11, Steven Aerts <steven.ae...@gmail.com> > wrote: > > > > > Xavier, Colin and Tom > > > > > > can you line up on this? > > > I don't really mind which solution is chosen, but I think it needs to > be > > > done be before I can close the vote. > > > > > > I want to help you with the implementation after a decision has been > > made. > > > Just let me know. > > > > > > > > > Thanks, > > > > > > > > > Steven > > > > > > Op di 12 dec. 2017 om 03:54 schreef Colin McCabe <cmcc...@apache.org>: > > > > > > > Thanks, Xavier.... we should definitely think about what happens when > > > > exceptions are thrown from these functions. > > > > > > > > I would suggest maybe we should just implement whenComplete, rather > > than > > > > exposing addWaiter. addWaiter was never intended as a public API, > and > > > > it's a little weird. whenComplete is nice because it supports > > chaining, > > > > and should be more familiar to users of other async APIs. > > > > > > > > best, > > > > Colin > > > > > > > > > > > > On Fri, Dec 8, 2017, at 16:26, Xavier Léauté wrote: > > > > > 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 > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
