Thanks Guozhang! 1. I agree `EmitConfig` is better than `WindowConfig` and option 2 modifies less places. What do you think of option 1 which doesn't change the current `windowedBy` api but configures `EmitConfig` separately. The benefit of option 1 is if we need to configure something else later, we don't need to pile them on `windowedBy` but can add separate APIs. 2. I added it to `Stores` mainly to conform to https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/kstream/internals/TimeWindowedKStreamImpl.java#L227-L231. But We can also create an internal API to do that without modifying `Stores`.
Hao On Mon, Mar 14, 2022 at 7:52 PM Guozhang Wang <wangg...@gmail.com> wrote: > Hello Hao, > > Thanks for the proposal, I have some preference among the options here so I > will copy them here: > > I'm now thinking if it's better to not add this new config on each of the > Window interfaces, but instead add that at the KGroupedStream#windowedBy > function. Also instead of adding just a boolean flag, maybe we can add a > Configured class like Grouped, Suppressed, etc, e.g. let's call it a > Emitted which for now would just have a single construct as > Emitted.atWindowClose whose semantics is the same as emitFinal == true. I > think the benefits are: > > 1) you do not need to modify multiple Window classes, but just overload one > windowedBy function with a second param. This is less of a scope for now, > and also more extensible for any future changes. > > 2) With a config interface, we maintain its extensibility as well as being > able to reuse this Emitted interface for other operators if we wanted to > expand to. > > ---------------------------- > > So in general I'm leaning towards option 2). For that, some more detailed > comments: > > 1) If we want to reuse that config object for other non-window stateful > operations, I think naming it as `EmitConfig` is probably better than > `WindowConfig`. > 2) I saw your PR (https://github.com/apache/kafka/pull/11892) that you are > also proposing to add new stores into the public factory Stores, but it's > not included in the KIP. Is that intentional? Personally I think that > although we may eventually want to add a new store type to the public APIs, > for this KIP maybe we do not have to add them but can delay for later after > we've learned the best way to layout. LMK what do you think? > > > > Guozhang > > > > On Fri, Mar 11, 2022 at 2:13 PM Hao Li <h...@confluent.io.invalid> wrote: > > > Hi Dev team, > > > > I'd like to start a discussion thread on Kafka Streams KIP-825: > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-825%3A+introduce+a+new+API+to+control+when+aggregated+results+are+produced > > > > This KIP is aimed to add new APIs to support outputting final aggregated > > results for windowed aggregations. I listed several options there and I'm > > looking forward to your feedback. > > > > Thanks, > > Hao > > > > > -- > -- Guozhang > -- Thanks, Hao
