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....@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 <matth...@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
