Hey Walker, Thanks for the KIP! I have just a couple of questions:
1) It seems a little awkward to me that with the current API, we have a nearly identical "add stream to cogroup" method, except for the first which has a different signature (ie the first stream is joined as stream.cogroup(Aggregator) while the subsequent ones are joined as .cogroup(Stream, Aggregator) ). I'm not sure what it would look like exactly, but I was just wondering if you'd considered and/or rejected any alternative APIs? 2) This might just be my lack of familiarity with "cogroup" as a concept, but with the current (non-optimal) API the user seems to have some control over how exactly the different streams are joined through the ValueJoiners. Would this new cogroup simply concatenate the values from the different cogroup streams, or could users potentially pass some kind of Joiner to the cogroup/aggregate methods? Or, is the whole point of cogroups that you no longer ever need to specify a Joiner? If so, you should add a short line to the KIP explaining that for those of us who aren't fluent in cogroup semantics :) Cheers, Sophie On Thu, Oct 17, 2019 at 3:06 PM Walker Carlson <wcarl...@confluent.io> wrote: > Good catch I updated that. > > I have made a PR for this KIP > > I then am splitting it into 3 parts, first cogroup for a key-value store ( > here <https://github.com/apache/kafka/pull/7538>), then for a > timeWindowedStore, and then a sessionWindowedStore + ensuring partitioning. > > Walker > > On Tue, Oct 15, 2019 at 12:47 PM Matthias J. Sax <matth...@confluent.io> > wrote: > > > Walker, > > > > thanks for picking up the KIP and reworking it for the changed API. > > > > Overall, the updated API suggestions make sense to me. The seem to align > > quite nicely with our current API design. > > > > One nit: In `CogroupedKStream#aggregate(...)` the type parameter of > > `Materialized` should be `V`, not `VR`? > > > > > > -Matthias > > > > > > > > On 10/14/19 2:57 PM, Walker Carlson wrote: > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-150+-+Kafka-Streams+Cogroup > > > here > > > is a link > > > > > > On Mon, Oct 14, 2019 at 2:52 PM Walker Carlson <wcarl...@confluent.io> > > > wrote: > > > > > >> Hello all, > > >> > > >> I have picked up and updated KIP-150. Due to changes to the project > > since > > >> KIP #150 was written there are a few items that need to be updated. > > >> > > >> First item that changed is the adoption of the Materialized parameter. > > >> > > >> The second item is the WindowedBy. How the old KIP handles windowing > is > > >> that it overloads the aggregate function to take in a Window object as > > well > > >> as the other parameters. The current practice to window > grouped-streams > > is > > >> to call windowedBy and receive a windowed stream object. The existing > > >> interface for a windowed stream made from a grouped stream will not > work > > >> for cogrouped streams. Hence, we have to make new interfaces for > > cogrouped > > >> windowed streams. > > >> > > >> Please take a look, I would like to hear your feedback, > > >> > > >> Walker > > >> > > > > > > > >
