You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Damian Guy <da...@gmail.com> on 2016/11/24 15:59:03 UTC
[DISCUSS] KIP-94: Session Windows
Hi all,
I would like to start the discussion on KIP-94:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-94+Session+Windows
Thanks,
Damian
Re: [DISCUSS] KIP-94: Session Windows
Posted by Guozhang Wang <wa...@gmail.com>.
1. Looks good. Thanks for the explanation. We can add some comments in the
javadoc that this is normally not needed for app programmers, but only for
the framework developers.
2. Thanks!
On Mon, Dec 5, 2016 at 2:21 AM, Damian Guy <da...@gmail.com> wrote:
> Hi Guozhang,
>
> Thanks for the feedback.
>
> 1. If we want developers to be able to provide custom SessionStore
> implementations then we need this method to be on the public interface. We
> are currently allowing this for other operations, so it seems to make
> sense, for the sake of consistency, to allow a custom SessionStore. What do
> you think?
>
> 2. Sure, makes sense. I'll update the KIP.
>
> Thanks,
> Damian
>
>
> On Sat, 3 Dec 2016 at 00:06 Guozhang Wang <wa...@gmail.com> wrote:
>
> > Thanks for the KIP Damian, it looks great to me overall. A couple of
> minor
> > comments:
> >
> > 1. findSessionsToMerge(): does this need to be a public interface? Does
> > users ever need to call it from the processor API or even DSL?
> >
> > 2. SessionMerger: I am wondering if we could make it more general by
> > renaming to "Merger" only? This if for future potential work when we have
> > alternative aggregation operator implementations where both an aggregator
> > V, T -> T and a merger T, T -> T could be provided by users.
> >
> >
> > Guozhang
> >
> >
> > On Wed, Nov 30, 2016 at 2:42 AM, Damian Guy <da...@gmail.com>
> wrote:
> >
> > > Thanks Matthias.
> > >
> > > 1) Yes good suggestion will update.
> > > 2) As it is consistent with Aggregator and a developer may want to use
> > the
> > > key. So why not?
> > > 3) Thanks. I'll update the KIP
> > >
> > > Cheers,
> > > Damian
> > >
> > > On Tue, 29 Nov 2016 at 23:47 Matthias J. Sax <ma...@confluent.io>
> > > wrote:
> > >
> > > > Very nice KIP!
> > > >
> > > >
> > > > Couple of comments:
> > > >
> > > > 1) Current window API is as follows:
> > > >
> > > > >
> > > > JoinWindows.of(timeDifference).before(timeDifference).after(
> > > timeDifference)
> > > > > TimeWindows.of(size).advanceBy(interval)
> > > > > UnlimitedWindow.of().startOn(start)
> > > >
> > > > To align with this scheme, I think it would be better to use the
> > > > following API for SessionWindows
> > > >
> > > > > SessionWindows.with(inactivityGap)
> > > >
> > > >
> > > > 2) I am wondering, why SessionMerger does need the key?
> > > >
> > > > 3) You KIP API for SessionWindows and you PR does not align. There
> are
> > > > some getters in you code that are not part of the KIP (not sure how
> > > > important this is)
> > > >
> > > >
> > > > -Matthias
> > > >
> > > >
> > > >
> > > > On 11/24/16 7:59 AM, Damian Guy wrote:
> > > > > Hi all,
> > > > >
> > > > > I would like to start the discussion on KIP-94:
> > > > >
> > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 94+Session+Windows
> > > > >
> > > > > Thanks,
> > > > > Damian
> > > > >
> > > >
> > > >
> > >
> >
> >
> >
> > --
> > -- Guozhang
> >
>
--
-- Guozhang
Re: [DISCUSS] KIP-94: Session Windows
Posted by Damian Guy <da...@gmail.com>.
Hi Guozhang,
Thanks for the feedback.
1. If we want developers to be able to provide custom SessionStore
implementations then we need this method to be on the public interface. We
are currently allowing this for other operations, so it seems to make
sense, for the sake of consistency, to allow a custom SessionStore. What do
you think?
2. Sure, makes sense. I'll update the KIP.
Thanks,
Damian
On Sat, 3 Dec 2016 at 00:06 Guozhang Wang <wa...@gmail.com> wrote:
> Thanks for the KIP Damian, it looks great to me overall. A couple of minor
> comments:
>
> 1. findSessionsToMerge(): does this need to be a public interface? Does
> users ever need to call it from the processor API or even DSL?
>
> 2. SessionMerger: I am wondering if we could make it more general by
> renaming to "Merger" only? This if for future potential work when we have
> alternative aggregation operator implementations where both an aggregator
> V, T -> T and a merger T, T -> T could be provided by users.
>
>
> Guozhang
>
>
> On Wed, Nov 30, 2016 at 2:42 AM, Damian Guy <da...@gmail.com> wrote:
>
> > Thanks Matthias.
> >
> > 1) Yes good suggestion will update.
> > 2) As it is consistent with Aggregator and a developer may want to use
> the
> > key. So why not?
> > 3) Thanks. I'll update the KIP
> >
> > Cheers,
> > Damian
> >
> > On Tue, 29 Nov 2016 at 23:47 Matthias J. Sax <ma...@confluent.io>
> > wrote:
> >
> > > Very nice KIP!
> > >
> > >
> > > Couple of comments:
> > >
> > > 1) Current window API is as follows:
> > >
> > > >
> > > JoinWindows.of(timeDifference).before(timeDifference).after(
> > timeDifference)
> > > > TimeWindows.of(size).advanceBy(interval)
> > > > UnlimitedWindow.of().startOn(start)
> > >
> > > To align with this scheme, I think it would be better to use the
> > > following API for SessionWindows
> > >
> > > > SessionWindows.with(inactivityGap)
> > >
> > >
> > > 2) I am wondering, why SessionMerger does need the key?
> > >
> > > 3) You KIP API for SessionWindows and you PR does not align. There are
> > > some getters in you code that are not part of the KIP (not sure how
> > > important this is)
> > >
> > >
> > > -Matthias
> > >
> > >
> > >
> > > On 11/24/16 7:59 AM, Damian Guy wrote:
> > > > Hi all,
> > > >
> > > > I would like to start the discussion on KIP-94:
> > > >
> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 94+Session+Windows
> > > >
> > > > Thanks,
> > > > Damian
> > > >
> > >
> > >
> >
>
>
>
> --
> -- Guozhang
>
Re: [DISCUSS] KIP-94: Session Windows
Posted by Guozhang Wang <wa...@gmail.com>.
Thanks for the KIP Damian, it looks great to me overall. A couple of minor
comments:
1. findSessionsToMerge(): does this need to be a public interface? Does
users ever need to call it from the processor API or even DSL?
2. SessionMerger: I am wondering if we could make it more general by
renaming to "Merger" only? This if for future potential work when we have
alternative aggregation operator implementations where both an aggregator
V, T -> T and a merger T, T -> T could be provided by users.
Guozhang
On Wed, Nov 30, 2016 at 2:42 AM, Damian Guy <da...@gmail.com> wrote:
> Thanks Matthias.
>
> 1) Yes good suggestion will update.
> 2) As it is consistent with Aggregator and a developer may want to use the
> key. So why not?
> 3) Thanks. I'll update the KIP
>
> Cheers,
> Damian
>
> On Tue, 29 Nov 2016 at 23:47 Matthias J. Sax <ma...@confluent.io>
> wrote:
>
> > Very nice KIP!
> >
> >
> > Couple of comments:
> >
> > 1) Current window API is as follows:
> >
> > >
> > JoinWindows.of(timeDifference).before(timeDifference).after(
> timeDifference)
> > > TimeWindows.of(size).advanceBy(interval)
> > > UnlimitedWindow.of().startOn(start)
> >
> > To align with this scheme, I think it would be better to use the
> > following API for SessionWindows
> >
> > > SessionWindows.with(inactivityGap)
> >
> >
> > 2) I am wondering, why SessionMerger does need the key?
> >
> > 3) You KIP API for SessionWindows and you PR does not align. There are
> > some getters in you code that are not part of the KIP (not sure how
> > important this is)
> >
> >
> > -Matthias
> >
> >
> >
> > On 11/24/16 7:59 AM, Damian Guy wrote:
> > > Hi all,
> > >
> > > I would like to start the discussion on KIP-94:
> > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 94+Session+Windows
> > >
> > > Thanks,
> > > Damian
> > >
> >
> >
>
--
-- Guozhang
Re: [DISCUSS] KIP-94: Session Windows
Posted by Damian Guy <da...@gmail.com>.
Thanks Matthias.
1) Yes good suggestion will update.
2) As it is consistent with Aggregator and a developer may want to use the
key. So why not?
3) Thanks. I'll update the KIP
Cheers,
Damian
On Tue, 29 Nov 2016 at 23:47 Matthias J. Sax <ma...@confluent.io> wrote:
> Very nice KIP!
>
>
> Couple of comments:
>
> 1) Current window API is as follows:
>
> >
> JoinWindows.of(timeDifference).before(timeDifference).after(timeDifference)
> > TimeWindows.of(size).advanceBy(interval)
> > UnlimitedWindow.of().startOn(start)
>
> To align with this scheme, I think it would be better to use the
> following API for SessionWindows
>
> > SessionWindows.with(inactivityGap)
>
>
> 2) I am wondering, why SessionMerger does need the key?
>
> 3) You KIP API for SessionWindows and you PR does not align. There are
> some getters in you code that are not part of the KIP (not sure how
> important this is)
>
>
> -Matthias
>
>
>
> On 11/24/16 7:59 AM, Damian Guy wrote:
> > Hi all,
> >
> > I would like to start the discussion on KIP-94:
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-94+Session+Windows
> >
> > Thanks,
> > Damian
> >
>
>
Re: [DISCUSS] KIP-94: Session Windows
Posted by "Matthias J. Sax" <ma...@confluent.io>.
Very nice KIP!
Couple of comments:
1) Current window API is as follows:
> JoinWindows.of(timeDifference).before(timeDifference).after(timeDifference)
> TimeWindows.of(size).advanceBy(interval)
> UnlimitedWindow.of().startOn(start)
To align with this scheme, I think it would be better to use the
following API for SessionWindows
> SessionWindows.with(inactivityGap)
2) I am wondering, why SessionMerger does need the key?
3) You KIP API for SessionWindows and you PR does not align. There are
some getters in you code that are not part of the KIP (not sure how
important this is)
-Matthias
On 11/24/16 7:59 AM, Damian Guy wrote:
> Hi all,
>
> I would like to start the discussion on KIP-94:
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-94+Session+Windows
>
> Thanks,
> Damian
>