Thanks lot for the KIP. The general idea to allow reading windowed-KTables is very useful! Couple of initial comments/question:
About only adding a single `windowedTable()` with no overloads: - retention time is no mandatory parameter and we can always use the default of 1 day - instead of enforcing both, Consumed and Materialized, we could also extend WindowedSerde to exposed its wrapped key-Serde; thus, if a user passes in `WindowedSerde` the inner key-serde can be extracted and users do not need to pass in the key-Serde explicitly. - while I agree that we need to pass in the window-size parameter, I am not sure if using Materialized is the best way to do this; it seems that window-size is the only mandatory parameter, thus we might be able to pass it directly and thus allow to make `Consumed` and `Materialized` optional. Something like: > windowedTable(String topicName, long windowSizeMs); As an (maybe better) alternative, we could also introduce a public `Windowed` interface similar to `Produced`, `Consumed`, `Materialized` etc that we us to pass in window parameters. Or maybe reuse the existing `Windows` class (ie, the same definition that is used in KGroupStream#windowedBy() can be passed into the new `windowedTable()` method. I also noticed, that the KIP only covers TimesWindows. Should we extend it to cover SessionWindows, too? > One side effect is that we bring `ChangeLoggingWindowBytesStore` public for unit test purpose. No need to mention this, because this class in in package "internals" and not part of public API. -Matthias On 5/24/18 10:39 PM, Boyang Chen wrote: > Hey friends, > > > I know this is critical time for the 2.0 release. Just want to call out again > for further review on the API format. Any feedback would be appreciated, > thank you! > > > Boyang > > ________________________________ > From: Liquan Pei <[email protected]> > Sent: Tuesday, May 22, 2018 4:29 AM > To: [email protected] > Subject: Re: [DISCUSSION] KIP-300: Add Windowed KTable API in StreamsBuilder > > This KIP makes sharing a WindowedKTable among Kafka Stream jobs very easy. > It would be nice to get this into trunk soon. > > Best, > Liquan > > On Mon, May 21, 2018 at 12:25 PM, Boyang Chen <[email protected]> wrote: > >> Hey all, >> >> >> I would like to start a discussion thread on KIP 300, which introduces a >> new API called windowedTable() in StreamsBuilder: >> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP- >> 300%3A+Add+Windowed+KTable+API+in+StreamsBuilder >> >> >> The pull request I'm working on is here: https://github.com/apache/ >> kafka/pull/5044 >> >> >> I understood that the community is busy working on 2.0 release, but this >> KIP is really important for our internal use case. So if any of you got >> time, please focus on clarifying the use case and reaching the agreement of >> API. Really appreciate your time! >> >> >> Best, >> >> Boyang >> >> >> > > > -- > Liquan Pei > Software Engineer, Confluent Inc >
signature.asc
Description: OpenPGP digital signature
