kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Guozhang Wang <wangg...@gmail.com>
Subject Re: [DISCUSS] KIP-94: Session Windows
Date Sat, 03 Dec 2016 00:06:00 GMT
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 <damian.guy@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 <matthias@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

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