Hi Leah, Thanks for the KIP! This has been a real pain for some use cases, so it's really good to see a proposal to fix it.
We do need a default constructor so that it can be dynamically instantiated by the consumer (or any other component). But I'm +1 on deprecating the constructor you're proposing to deprecate, which only partially configures the class. It seems like there are exactly two patterns: either you fully configure the class in the constructor and don't call `init()`, or you call the default constructor and then configure the class by calling `init()`. I can appreciate Walker's point, but stepping back, it doesn't actually seem that useful to partially configure the class in the constructor and then finish up the configuration by calling `init()`. I could see the argument if there were a sensible default, but for this particular class, there isn't one. Rhetorical question: what is the default window size for Streams programs? I have a couple of concerns to discuss: 1. Config Location I don't think I would add the new configs to ConsumerConfig, but would add it to StreamsConfig instead. The deserailzier itself is in Streams (it is o.a.k.streams.kstream.TimeWindowedDeserializer), so it seems odd to have one of its configurations in a completely different module. Also, this class already has two configs, which are in StreamsConfig: StreamsConfig.DEFAULT_WINDOWED_KEY_SERDE_INNER_CLASS StreamsConfig.DEFAULT_WINDOWED_VALUE_SERDE_INNER_CLASS It seems like the new config belongs right next to the existing ones. For me, it raises a secondary question: 1b: Should there be a KEY_WINDOW_SIZE and a VALUE_WINDOW_SIZE? I'm honestly not sure what a "windowed value" even is, but the fact that we can configure serdes for it implies that perhaps we should symmetrically configure its size as well. 2. Fixed Size Assumption In KIP-645, I'm proposing to lift the assumption that TimeWindows have a fixed size at all, but KIP-659 is currently built on that assumption. For details on why this is not a good assumtion, see: https://issues.apache.org/jira/browse/KAFKA-10408 In fact, in my POC PR for KIP-659, I'm dropping the constructor that takes a "window size" parameter in favor of one that takes a window function, mapping a window start time to a full Window(start, end). In that context, it seems incongruous to introduce a configuration that specifies a window size. Of course, my KIP is also under discussion, so my proposal may not eventually be accepted. But it is necessary to consider both of these concerns together. One option seems to be to accept both. Namely, we keep the "fixed size" constructor AND add my new constructor (for variably sized windows). Likewise, we accept your proposal, and KIP-659 would propose to add a new config specifying a windowing function, such as: > StreamsConfig.WINDOW_FUNCTION_CONFIG which would be an instance of: > public interface WindowFunction implements Function<Long, Window>; I'm not bringing these up for discussion in your KIP right now, just demonstrating the feasibility of merging both proposals. My question for you: do you think the general strategy of having two constructors and two configs, one for fixed and one for variable windows, makes sense? Is it too complicated? Do you have a better idea? Thanks! -John On Thu, 2020-08-20 at 14:49 -0700, Walker Carlson wrote: > Hi Leah, > > Could you explain a bit more why we do not wish to > let TimeWindowedDeserializer and WindowedSerdes be created without a > specified time as a parameter? > > I understand the long.MAX_VALUE could cause problems but would it not be a > good idea to have a usable default or fetch from the config if available? > After all you are proposing to add "window.size.ms" > > We definitely need a fix to this problem and adding "window.size.ms" makes > sense to me. > > Thanks for the KIP, > Walker > > On Thu, Aug 20, 2020 at 2:22 PM Leah Thomas <ltho...@confluent.io> wrote: > > > Hi all, > > > > I'd like to start a discussion for KIP-659: > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-659%3A+Improve+TimeWindowedDeserializer+and+TimeWindowedSerde+to+handle+window+size > > > > > > The goal of the KIP is to ensure that window size is passed to the consumer > > when needed, which will generally be for testing purposes, and to avoid > > runtime errors when the *TimeWindowedSerde* is created without a window > > size. > > > > Looking forward to hearing your feedback. > > > > Cheers, > > Leah > >
