Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Leah Thomas]

Thanks everyone, with that I'll go ahead and close this KIP with 4 binding
votes (Sophie, Guozhang, John, Matthias). Thanks for the lively discussion!

Leah

On Wed, Jan 20, 2021 at 6:00 PM Matthias J. Sax  wrote:

> Thanks for reviving this KIP, Leah.
>
> I agree that we should not extend the scope of this KIP to potentially
> deprecate/rename the `default.windowed.[key|value].serde.inner` configs.
>
> @Sophie: if you feel strong about it, let's do a separate KIP.
>
>
> +1 (binding)
>
>
> -Matthias
>
> On 1/19/21 3:00 PM, John Roesler wrote:
> > Hi all,
> >
> > I've just caught up on the thread, and FWIW, I'm still +1.
> >
> > Thanks,
> > -John
> >
> > On Mon, 2021-01-18 at 21:53 -0800, Guozhang Wang wrote:
> >> 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 
> 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
> >
> 

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Matthias J. Sax]

Thanks for reviving this KIP, Leah.

I agree that we should not extend the scope of this KIP to potentially
deprecate/rename the `default.windowed.[key|value].serde.inner` configs.

@Sophie: if you feel strong about it, let's do a separate KIP.


+1 (binding)


-Matthias

On 1/19/21 3:00 PM, John Roesler wrote:
> Hi all,
> 
> I've just caught up on the thread, and FWIW, I'm still +1.
> 
> Thanks,
> -John
> 
> On Mon, 2021-01-18 at 21:53 -0800, Guozhang Wang wrote:
>> 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 
>> wrote:
>>
>>> That sounds good to me. Thanks for reviving this
>>>
>>> Sophie
>>>
>>> On Wed, Jan 13, 2021 at 7:47 AM Leah Thomas  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 
>>> 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 

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[John Roesler]

Hi all,

I've just caught up on the thread, and FWIW, I'm still +1.

Thanks,
-John

On Mon, 2021-01-18 at 21:53 -0800, Guozhang Wang wrote:
> 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 
> wrote:
> 
> > That sounds good to me. Thanks for reviving this
> > 
> > Sophie
> > 
> > On Wed, Jan 13, 2021 at 7:47 AM Leah Thomas  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 
> > 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 

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Guozhang Wang]

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 
wrote:

> That sounds good to me. Thanks for reviving this
>
> Sophie
>
> On Wed, Jan 13, 2021 at 7:47 AM Leah Thomas  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 
> 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 

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Sophie Blee-Goldman]

That sounds good to me. Thanks for reviving this

Sophie

On Wed, Jan 13, 2021 at 7:47 AM Leah Thomas  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 
> 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  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 

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Leah Thomas]

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 
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  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 

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Sophie Blee-Goldman]

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  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  >
> > 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 

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Matthias J. Sax]

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 
> 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  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 
>> 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 
>> 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 
>>> wrote:
>
>> +1 (binding)
>>
>> 

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Guozhang Wang]

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 
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  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 
> 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 
> 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 
> > 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,
> > > 

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Leah Thomas]

Hey Sophie,

Are you advocating that we change the name back to *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
* instead of default?

Cheers,
Leah

On Tue, Sep 8, 2020 at 12:50 PM Sophie Blee-Goldman 
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  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 
> 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 
> 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 
> > 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
> > > > > 

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Sophie Blee-Goldman]

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  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  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  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 
> 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  >
> > > > 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
> >
>

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Guozhang Wang]

Thanks Leah, I'm +1 on the KIP proposal.


Guozhang

On Tue, Sep 8, 2020 at 10:40 AM Leah Thomas  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  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  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 
> 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  >
> > > > 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

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Leah Thomas]

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  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  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  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 
> > > 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
>

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Guozhang Wang]

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  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  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 
> > 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

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Leah Thomas]

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  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 
> 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
> 
> >
>
>

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Matthias J. Sax]

+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  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

> 



signature.asc
Description: OpenPGP digital signature

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[John Roesler]

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  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
> > > 

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Sophie Blee-Goldman]

Thanks for the KIP! +1 (non-binding)

Sophie

On Mon, Aug 24, 2020 at 5:06 PM John Roesler  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
> >
>

Re: [VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[John Roesler]

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
>

[VOTE] KIP-659: Improve TimeWindowedDeserializer and TimeWindowedSerde to handle window size

[Leah Thomas]

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