kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ismael Juma <ism...@juma.me.uk>
Subject Re: [DISCUSS] KIP-218: Make KafkaFuture.Function java 8 lambda compatible
Date Tue, 19 Dec 2017 08:42:50 GMT
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é <xavier@confluent.io> wrote:

> I'm fine with the whenComplete solution as well.
> On Tue, Dec 12, 2017 at 03:57 Tom Bentley <t.j.bentley@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.aerts@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 <cmccabe@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.aerts@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.bentley@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.aerts@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.bentley@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.aerts@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 <
> > > > ben@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 <
> > > > cmccabe@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.bentley@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 <
> > > > yuzhihong@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 <
> > > > > > > > ismael@juma.me.uk>
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > >>
> > > > > > > > > > > > > >> > On Wed, Nov
1, 2017 at 3:51 PM, Ted Yu <
> > > > > > > yuzhihong@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
> > > > > > > > > > > > > >> >
> > > > > > > > > > > > > >>
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message