Hey Sophie, Are you advocating that we change the name back to *window.size.ms <http://window.size.ms>*?
I wasn't thinking that by including the config we would steer people away from using the constructors that pass in a window size, so in that sense I suppose the config isn't "default." I think that by deprecating the TimeWindowedSerde(innerClass) constructor, as you mentioned, we'll steer clear from some of the issues you brought up. In my mind, the first tier of usage is to set the window size directly, and then after that is to use the config, and if neither of those happen, then we would fall back on Long.MAX_VALUE. I suppose in this sense, "default" is an imprecise term, but indicates that if the proper constructor/set up is not used, there will be a "default" window size that the user still sets. It seems like the console consumer has us somewhat stuck with a new config, but I agree that the new constructors should be relied upon before the config is. On that line, what about a name like *backup.window.size.ms <http://backup.window.size.ms>* instead of default? Cheers, Leah On Tue, Sep 8, 2020 at 12:50 PM 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 > > > > > >
