Read the above updates and the KIP's scope. Makes sense to me. +1 still counts :)
On Wed, Jan 13, 2021 at 2:04 PM Sophie Blee-Goldman <sop...@confluent.io> wrote: > That sounds good to me. Thanks for reviving this > > Sophie > > On Wed, Jan 13, 2021 at 7:47 AM Leah Thomas <ltho...@confluent.io> wrote: > > > Hey all, > > > > Bringing this back up for discussion. > > > > It seems like the next steps are to: > > 1. rename the config "window.size.ms" > > 2. ensure that users set window size EITHER through the config OR through > > the constructor. On this note, it may make sense to remove the default > for > > the `window.size.ms` config, so that there won't be a fall back if the > > window size isn't set in either spot. WDYT? This could also address the > > issue of multiple window sizes within a streams app. > > > > I see what Sophie is saying about the `default.windowed.key.serde.inner` > > config, but I do think deprecating and moving those configs would > require a > > larger discussion. I'm open to looping them into this KIP if we feel like > > it's vital (or incredibly convenient with low cost to users), but my > > initial reaction is to leave that out for now and work within the current > > set-up for window size. > > > > Thanks for all the comments so far, > > Leah > > > > On Tue, Sep 29, 2020 at 10:44 PM Sophie Blee-Goldman < > sop...@confluent.io> > > wrote: > > > > > There are two cases where you need to specify the window size -- > directly > > > using a > > > Consumer (eg the console consumer) or reading as an input topic within > > > Streams. > > > We need a config for the first case, since you can't pass a > Deserializer > > > object to the > > > console consumer. In the Streams case, the reverse is true, and you > have > > to > > > pass in > > > an actual Serde object. > > > > > > Imo we should keep these two cases separate and not use the config for > > the > > > Streams > > > case at all. But that's hard to enforce (we'd have to strip the config > > out > > > of the user's > > > StreamsConfig if they tried to use it within Streams, for example) and > it > > > also puts us > > > in an awkward position due to the default.windowed.inner.serde.class > > > configs. If > > > they can specify the inner serde class through their Streams app > config, > > > they > > > should be able to specify the window size through config as well. > > Otherwise > > > we > > > either force a mix-and-match as Matthias described, or you just always > > have > > > to > > > specify both the inner class and the window size in the constructor, at > > > which > > > point, why even have the default.windowed.inner.serde.class config at > > all? > > > > > > ... > > > that's not a rhetorical question, actually. Personally I do think we > > should > > > deprecate the default.windowed.serde.inner.class and replace it with > > > separate > > > windowed.serializer.inner.class/windowed.deserializer.inner.class > > configs. > > > This > > > way it's immediately obvious that the configs are only for the > > > Consumer/Producer, > > > and you should construct your own TimeWindowedSerde with all the > > necessary > > > parameters for use in your Streams app. > > > > > > That might be too radical, and maybe the problem isn't worth the burden > > of > > > forcing users to change their code and replace the config with actual > > Serde > > > objects. But it should be an easy change to make, and if it isn't, > that's > > > probably a good sign that you're using the serde incorrectly somewhere. > > > > > > If we don't deprecate the default.windowed.serde.inner.class, then it's > > > less clear > > > to me how to proceed. The only really consistent thing to do seems to > be > > to > > > name and position the new window size config as a default config and > > allow > > > it to be used similar to the default inner class configs. Which, as > > > established > > > throughout this discussion, seems very very wrong > > > > > > So yes, I think we should just stick with the original name > > window.size.ms > > > . > > > Or > > > better yet, call it windowed.deserializer.window.size.ms and name the > > > default.windowed.serde.inner.class replacements > > > windowed.deserializer.inner.class and windowed.serializer.inner.class > > > > > > On Tue, Sep 8, 2020 at 2:07 PM Matthias J. Sax <mj...@apache.org> > wrote: > > > > > > > From my understanding, the KIP aims for the case when a user does not > > > > control the code, eg, when using the command line consumer (or > similar > > > > tools). > > > > > > > > If the user writes code, we should always encourage her to > instantiate > > > > the deserializer explicitly and not relying on reflection+config. > > > > > > > > I also don't think that the `default` prefix does make sense, as it > > > > indicates that there might be a non-default. However, IMHO, we should > > > > not allow "overwrite semantics" but rather throw an exception if the > > > > config is set and a window size is provided via the constructor. We > > > > should not allow to mix-and-match both and should stick to a strict > > > > either-or pattern. > > > > > > > > > > > > -Matthias > > > > > > > > On 9/8/20 11:52 AM, Guozhang Wang wrote: > > > > > Hi Sophie, > > > > > > > > > > Seems I do have some mis-understanding of the KIP's motivation here > > :) > > > > Just > > > > > for clarification my reasoning is that: > > > > > > > > > > 1) today Streams itself never uses a windowed deserializer itself > > since > > > > its > > > > > built-in operators only need the serializer and users do not need > to > > > > > override it, plus standby / restore active tasks would just copy > the > > > > bytes > > > > > directly. So this KIP's motivation is not for Stream's own code > > > anyways. > > > > > > > > > > 2) It is only when user specified serde is missing the window size, > > > which > > > > > is either when a) one is trying to read a source topic as windowed > > > > records > > > > > in Streams, this is a big blocker for KIP-300, and when b) one is > > > trying > > > > to > > > > > read a topic as windowed records in Consumer, we would have issues > if > > > > users > > > > > fail to use the appropriate serde constructs. > > > > > > > > > > I thought the main motivation of this KIP is for 2.a), in which we > > > would > > > > > just encourage the users to use the right constructor with the > window > > > > size > > > > > by deprecating the other constructs. But I'm not sure how this > would > > > help > > > > > with 2.b) since the proposal is on adding to StreamsConfig. If it > is > > > the > > > > > case, then I agree that probably we can just not add an extra > config > > > but > > > > > just deprecating the constructs. > > > > > > > > > > > > > > > Guozhang > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Sep 8, 2020 at 10:50 AM Sophie Blee-Goldman < > > > sop...@confluent.io > > > > > > > > > > wrote: > > > > > > > > > >> Hey Guozhang & Leah, > > > > >> > > > > >> I want to push back a bit on the assumption that we would fall > back > > on > > > > this > > > > >> config > > > > >> in the case of an unspecified window size in a Streams serde. I > > don't > > > > think > > > > >> it should > > > > >> be a default at all, either in name or in effect. To borrow the > > > > rhetorical > > > > >> question that > > > > >> John raised earlier: what is the default window size of an > > > application? > > > > >> > > > > >> Personally, I agree that that doesn't make much sense. Sure, if > you > > > only > > > > >> have a single > > > > >> windowed operation in your app then you could just specify the > > window > > > > size > > > > >> by config, > > > > >> but how is that any more ergonomic than specifying the window size > > in > > > > the > > > > >> Serde's > > > > >> constructor? If anything, it seems worse to put physical and > mental > > > > >> distance between > > > > >> the specification and the actual usage of such parameters. What if > > you > > > > add > > > > >> another > > > > >> windowed operation later, with a different size, and forget to > > specify > > > > the > > > > >> new size in > > > > >> the new Serde? Or what if you never specify a default window size > > > > config at > > > > >> all and > > > > >> accidentally end up using the default config value of > > Long.MAX_VALUE? > > > > >> Avoiding this > > > > >> possibility was/is one of the main motivations of this KIP, and > the > > > > whole > > > > >> point of > > > > >> deprecating the TimeWindowedSerde(innerClass) constructor. > > > > >> > > > > >> I actually would have advocated to remove this config entirely, > but > > as > > > > John > > > > >> pointed > > > > >> out, we still need it to configure things like the console > consumer > > > > >> > > > > >> On Tue, Sep 8, 2020 at 10:40 AM Leah Thomas <ltho...@confluent.io > > > > > > wrote: > > > > >> > > > > >>> Hi Guozhang, > > > > >>> > > > > >>> Yes, the config would read them as a single window size. I think > > this > > > > >>> relates to John's comments about having variably sized windows, > > which > > > > >> this > > > > >>> config doesn't handle. I like the name change and updated the > wiki > > to > > > > >>> reflect that, and to clarify that the default value will still be > > > > >>> Long.MAX_VALUE. > > > > >>> > > > > >>> Thanks for your feedback! > > > > >>> Leah > > > > >>> > > > > >>> On Tue, Sep 8, 2020 at 11:54 AM Guozhang Wang < > wangg...@gmail.com> > > > > >> wrote: > > > > >>> > > > > >>>> Hello Leah, > > > > >>>> > > > > >>>> Thanks for initiating this. I just have one minor clarification > > > > >> question > > > > >>>> here: the config "window.size.ms" seems to be used as the > default > > > > >> window > > > > >>>> size when reading from a topic that represents windowed records > > > right? > > > > >>> I.e. > > > > >>>> if there are multiple topics that represent windowed records but > > > their > > > > >>>> window sizes are different, with this config we can only read > them > > > > >> with a > > > > >>>> single window size? If yes, could we rename the config as " > > > > >>>> default.window.size.ms" and make that clear in the description > as > > > > >> well? > > > > >>>> Also we'd better also include its default value which I think > > would > > > > >> still > > > > >>>> be MAX_VALUE for compatibility. > > > > >>>> > > > > >>>> > > > > >>>> Guozhang > > > > >>>> > > > > >>>> > > > > >>>> On Tue, Sep 8, 2020 at 9:38 AM Leah Thomas < > ltho...@confluent.io> > > > > >> wrote: > > > > >>>> > > > > >>>>> Hey all, > > > > >>>>> > > > > >>>>> We should be good to wrap up voting now that the discussion has > > > been > > > > >>>>> resolved. > > > > >>>>> > > > > >>>>> Cheers, > > > > >>>>> Leah > > > > >>>>> > > > > >>>>> On Wed, Sep 2, 2020 at 7:23 PM Matthias J. Sax < > mj...@apache.org > > > > > > > >>> wrote: > > > > >>>>> > > > > >>>>>> +1 (binding) > > > > >>>>>> > > > > >>>>>> On 8/26/20 8:02 AM, John Roesler wrote: > > > > >>>>>>> Hi all, > > > > >>>>>>> > > > > >>>>>>> I've just sent a new message to the DISCUSS thread. We > > > > >>>>>>> forgot to include the Scala API in the proposal. > > > > >>>>>>> > > > > >>>>>>> Thanks, > > > > >>>>>>> -John > > > > >>>>>>> > > > > >>>>>>> On Mon, 2020-08-24 at 18:00 -0700, Sophie Blee-Goldman > > > > >>>>>>> wrote: > > > > >>>>>>>> Thanks for the KIP! +1 (non-binding) > > > > >>>>>>>> > > > > >>>>>>>> Sophie > > > > >>>>>>>> > > > > >>>>>>>> On Mon, Aug 24, 2020 at 5:06 PM John Roesler < > > > > >> vvcep...@apache.org > > > > >>>> > > > > >>>>>> wrote: > > > > >>>>>>>> > > > > >>>>>>>>> Thanks Leah, > > > > >>>>>>>>> I’m +1 (binding) > > > > >>>>>>>>> > > > > >>>>>>>>> -John > > > > >>>>>>>>> > > > > >>>>>>>>> On Mon, Aug 24, 2020, at 16:54, Leah Thomas wrote: > > > > >>>>>>>>>> Hi everyone, > > > > >>>>>>>>>> > > > > >>>>>>>>>> I'd like to kick-off the vote for KIP-659: Improve > > > > >>>>>>>>>> TimeWindowedDeserializer > > > > >>>>>>>>>> and TimeWindowedSerde to handle window size. > > > > >>>>>>>>>> > > > > >>>>>>>>> > > > > >>>>>> > > > > >>>>> > > > > >>>> > > > > >>> > > > > >> > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-659%3A+Improve+TimeWindowedDeserializer+and+TimeWindowedSerde+to+handle+window+size > > > > >>>>>>>>>> Thanks, > > > > >>>>>>>>>> Leah > > > > >>>>>>>>>> > > > > >>>>>>> > > > > >>>>>> > > > > >>>>>> > > > > >>>>> > > > > >>>> > > > > >>>> > > > > >>>> -- > > > > >>>> -- Guozhang > > > > >>>> > > > > >>> > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > -- -- Guozhang
