Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Sophie Blee-Goldman]

Thanks Matthias -- sorry I forgot to get back to you!

Thanks for kicking off the voting thread on this

On Thu, Mar 28, 2024 at 3:58 AM Matthias J. Sax  wrote:

> Thanks. I think you can start a VOTE.
>
> -Matthias
>
>
> On 3/20/24 9:24 PM, Lucia Cerchie wrote:
> > thanks Sophie, I've made the updates, would appreciate one more look
> before
> > submission
> >
> > On Wed, Mar 20, 2024 at 8:36 AM Sophie Blee-Goldman <
> sop...@responsive.dev>
> > wrote:
> >
> >> A few minor notes on the KIP but otherwise I think you can go ahead and
> >> call for a vote!
> >>
> >> 1. Need to update the Motivation section to say "console clients" or
> >> "console consumer/producer" instead of " plain consumer client"
> >> 2. Remove the first paragraph under "Public Interfaces" (ie the
> KIP-writing
> >> instructions) and also list the new config definitions here. You can
> just
> >> add a code snippet for each class (TimeWindowedDe/Serializer) with the
> >> actual variable definition you'll be adding. Maybe also add a code
> snippet
> >> for StreamsConfig with the @Deprecated annotation added to the two
> configs
> >> we're deprecating
> >> 3. nit: remove the "questions" under "Compatibility, Deprecation, and
> >> Migration Plan", ie the stuff from the KIP-writing template. Just makes
> it
> >> easier to read
> >> 4. In "Test Plan" we should also have a unit test to make sure the new "
> >> windowed.inner.de/serializer.class" takes preference in the case that
> both
> >> it and the old "windowed.inner.serde.class" config are both specified
> >>
> >> Also, this is more of a question than a suggestion, but is the KIP title
> >> perhaps a bit misleading in that people might assume these configs will
> no
> >> longer be available for use at all? What do people think about changing
> it
> >> to something like
> >>
> >> *Move `window.size.ms ` and
> >> `windowed.inner.serde.class` from `StreamsConfig` to
> >> TimeWindowedDe/Serializer class*
> >>
> >> A bit long/clunky but at least it gets the point across about where
> folks
> >> can find these configs going forward.
> >>
> >> On Mon, Mar 18, 2024 at 6:26 PM Lucia Cerchie
> >> 
> >> wrote:
> >>
> >>> Thanks for the discussion!
> >>>
> >>> I've updated the KIP here with what I believe are the relevant pieces,
> >>> please let me know if anything is missing:
> >>>
> >>
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=290982804
> >>>
> >>> On Sun, Mar 17, 2024 at 7:09 PM Sophie Blee-Goldman <
> >> sop...@responsive.dev
> 
> >>> wrote:
> >>>
>  Sounds good!
> 
>  @Lucia when you have a moment can you update the KIP with
>  the new proposal, including the details that Matthias pointed
>  out in his last response? After that's done I think you can go
>  ahead and call for a vote whenever you're ready!
> 
>  On Sat, Mar 16, 2024 at 7:35 PM Matthias J. Sax 
> >>> wrote:
> 
> > Thanks for the summary. Sounds right to me. That is what I would
> >>> propose.
> >
> > As you pointed out, we of course still need to support the current
> > confis, and we should log a warning when in use (even if the new one
> >> is
> > in use IMHO) -- but that's more an implementation detail.
> >
> > I agree that the new config should take preference in case both are
> > specified. This should be pointed out in the KIP, as it's an
> >> important
> > contract the user needs to understand.
> >
> >
> > -Matthias
> >
> > On 3/14/24 6:18 PM, Sophie Blee-Goldman wrote:
> >>>
> >>> Should we change it do `.serializer` and `.deserialize`?
> >>
> >> That's a good point -- if we're going to split this up by defining
> >>> the
> >> config
> >> in both the TimeWindowedSerializer and TimeWindowedDeserializer,
> >> then it makes perfect sense to go a step further and actually
> >> define
> >> only the relevant de/serializer class instead of the full serde
> >>
> >> Just to put this all together, it sounds like the proposal is to:
> >>
> >> 1) Deprecate both these configs where they appear in StreamsConfig
> >> (as per the original plan in the KIP, just reiterating it here)
> >>
> >> 2) Don't "define" either config in any specific client's Config
> >>> class,
> >> but just define a String variable with the config name in the
> >>> relevant
> >> de/serializer class, and maybe point people to it in the docs
> >>> somewhere
> >>
> >> 3) We would add three new public String variables for three
> >> different
> >> configs across two classes, specifically:
> >>
> >> In TimeWindowedSerializer:
> >> - define a constant for "windowed.inner.serializer.class"
> >> In TimeWindowedDeserializer:
> >> - define a constant for "windowed.inner.deserializer.class"
> >> - define a constant for "window.size.ms"
> >>
> >> 4) Lastly, we would update the windowed de/serializer
> >> 

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Matthias J. Sax]

Thanks. I think you can start a VOTE.

-Matthias


On 3/20/24 9:24 PM, Lucia Cerchie wrote:

thanks Sophie, I've made the updates, would appreciate one more look before
submission

On Wed, Mar 20, 2024 at 8:36 AM Sophie Blee-Goldman 
wrote:


A few minor notes on the KIP but otherwise I think you can go ahead and
call for a vote!

1. Need to update the Motivation section to say "console clients" or
"console consumer/producer" instead of " plain consumer client"
2. Remove the first paragraph under "Public Interfaces" (ie the KIP-writing
instructions) and also list the new config definitions here. You can just
add a code snippet for each class (TimeWindowedDe/Serializer) with the
actual variable definition you'll be adding. Maybe also add a code snippet
for StreamsConfig with the @Deprecated annotation added to the two configs
we're deprecating
3. nit: remove the "questions" under "Compatibility, Deprecation, and
Migration Plan", ie the stuff from the KIP-writing template. Just makes it
easier to read
4. In "Test Plan" we should also have a unit test to make sure the new "
windowed.inner.de/serializer.class" takes preference in the case that both
it and the old "windowed.inner.serde.class" config are both specified

Also, this is more of a question than a suggestion, but is the KIP title
perhaps a bit misleading in that people might assume these configs will no
longer be available for use at all? What do people think about changing it
to something like

*Move `window.size.ms ` and
`windowed.inner.serde.class` from `StreamsConfig` to
TimeWindowedDe/Serializer class*

A bit long/clunky but at least it gets the point across about where folks
can find these configs going forward.

On Mon, Mar 18, 2024 at 6:26 PM Lucia Cerchie

wrote:


Thanks for the discussion!

I've updated the KIP here with what I believe are the relevant pieces,
please let me know if anything is missing:


https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=290982804


On Sun, Mar 17, 2024 at 7:09 PM Sophie Blee-Goldman <

sop...@responsive.dev



wrote:


Sounds good!

@Lucia when you have a moment can you update the KIP with
the new proposal, including the details that Matthias pointed
out in his last response? After that's done I think you can go
ahead and call for a vote whenever you're ready!

On Sat, Mar 16, 2024 at 7:35 PM Matthias J. Sax 

wrote:



Thanks for the summary. Sounds right to me. That is what I would

propose.


As you pointed out, we of course still need to support the current
confis, and we should log a warning when in use (even if the new one

is

in use IMHO) -- but that's more an implementation detail.

I agree that the new config should take preference in case both are
specified. This should be pointed out in the KIP, as it's an

important

contract the user needs to understand.


-Matthias

On 3/14/24 6:18 PM, Sophie Blee-Goldman wrote:


Should we change it do `.serializer` and `.deserialize`?


That's a good point -- if we're going to split this up by defining

the

config
in both the TimeWindowedSerializer and TimeWindowedDeserializer,
then it makes perfect sense to go a step further and actually

define

only the relevant de/serializer class instead of the full serde

Just to put this all together, it sounds like the proposal is to:

1) Deprecate both these configs where they appear in StreamsConfig
(as per the original plan in the KIP, just reiterating it here)

2) Don't "define" either config in any specific client's Config

class,

but just define a String variable with the config name in the

relevant

de/serializer class, and maybe point people to it in the docs

somewhere


3) We would add three new public String variables for three

different

configs across two classes, specifically:

In TimeWindowedSerializer:
- define a constant for "windowed.inner.serializer.class"
In TimeWindowedDeserializer:
- define a constant for "windowed.inner.deserializer.class"
- define a constant for "window.size.ms"

4) Lastly, we would update the windowed de/serializer

implementations

to check for the new configs (ie "

windowed.inner.de/serializer.class

")

and use the provided de/serializer class, if one was given. If the

new

configs are not present, they would fall back to the

original/current

logic (ie that based on the old "windowed.inner.serde.class"

config)


I think that's everything. Does this sound about right for where we

want

to go with these configs?

On Thu, Mar 14, 2024 at 4:58 PM Matthias J. Sax 

wrote:



By "don't add them" do you just mean we would not have any

actual

variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
and simply document -- somewhere -- that one can use the string
"window.size.ms" when configuring a command-line client with a
windowed serde?


Yes. That's the idea.




I assume you aren't proposing

to remove the ability to use and understand this config from the
implementations themselves, but correct me if 

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Lucia Cerchie]

thanks Sophie, I've made the updates, would appreciate one more look before
submission

On Wed, Mar 20, 2024 at 8:36 AM Sophie Blee-Goldman 
wrote:

> A few minor notes on the KIP but otherwise I think you can go ahead and
> call for a vote!
>
> 1. Need to update the Motivation section to say "console clients" or
> "console consumer/producer" instead of " plain consumer client"
> 2. Remove the first paragraph under "Public Interfaces" (ie the KIP-writing
> instructions) and also list the new config definitions here. You can just
> add a code snippet for each class (TimeWindowedDe/Serializer) with the
> actual variable definition you'll be adding. Maybe also add a code snippet
> for StreamsConfig with the @Deprecated annotation added to the two configs
> we're deprecating
> 3. nit: remove the "questions" under "Compatibility, Deprecation, and
> Migration Plan", ie the stuff from the KIP-writing template. Just makes it
> easier to read
> 4. In "Test Plan" we should also have a unit test to make sure the new "
> windowed.inner.de/serializer.class" takes preference in the case that both
> it and the old "windowed.inner.serde.class" config are both specified
>
> Also, this is more of a question than a suggestion, but is the KIP title
> perhaps a bit misleading in that people might assume these configs will no
> longer be available for use at all? What do people think about changing it
> to something like
>
> *Move `window.size.ms ` and
> `windowed.inner.serde.class` from `StreamsConfig` to
> TimeWindowedDe/Serializer class*
>
> A bit long/clunky but at least it gets the point across about where folks
> can find these configs going forward.
>
> On Mon, Mar 18, 2024 at 6:26 PM Lucia Cerchie
> 
> wrote:
>
> > Thanks for the discussion!
> >
> > I've updated the KIP here with what I believe are the relevant pieces,
> > please let me know if anything is missing:
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=290982804
> >
> > On Sun, Mar 17, 2024 at 7:09 PM Sophie Blee-Goldman <
> sop...@responsive.dev
> > >
> > wrote:
> >
> > > Sounds good!
> > >
> > > @Lucia when you have a moment can you update the KIP with
> > > the new proposal, including the details that Matthias pointed
> > > out in his last response? After that's done I think you can go
> > > ahead and call for a vote whenever you're ready!
> > >
> > > On Sat, Mar 16, 2024 at 7:35 PM Matthias J. Sax 
> > wrote:
> > >
> > > > Thanks for the summary. Sounds right to me. That is what I would
> > propose.
> > > >
> > > > As you pointed out, we of course still need to support the current
> > > > confis, and we should log a warning when in use (even if the new one
> is
> > > > in use IMHO) -- but that's more an implementation detail.
> > > >
> > > > I agree that the new config should take preference in case both are
> > > > specified. This should be pointed out in the KIP, as it's an
> important
> > > > contract the user needs to understand.
> > > >
> > > >
> > > > -Matthias
> > > >
> > > > On 3/14/24 6:18 PM, Sophie Blee-Goldman wrote:
> > > > >>
> > > > >> Should we change it do `.serializer` and `.deserialize`?
> > > > >
> > > > > That's a good point -- if we're going to split this up by defining
> > the
> > > > > config
> > > > > in both the TimeWindowedSerializer and TimeWindowedDeserializer,
> > > > > then it makes perfect sense to go a step further and actually
> define
> > > > > only the relevant de/serializer class instead of the full serde
> > > > >
> > > > > Just to put this all together, it sounds like the proposal is to:
> > > > >
> > > > > 1) Deprecate both these configs where they appear in StreamsConfig
> > > > > (as per the original plan in the KIP, just reiterating it here)
> > > > >
> > > > > 2) Don't "define" either config in any specific client's Config
> > class,
> > > > > but just define a String variable with the config name in the
> > relevant
> > > > > de/serializer class, and maybe point people to it in the docs
> > somewhere
> > > > >
> > > > > 3) We would add three new public String variables for three
> different
> > > > > configs across two classes, specifically:
> > > > >
> > > > > In TimeWindowedSerializer:
> > > > >- define a constant for "windowed.inner.serializer.class"
> > > > > In TimeWindowedDeserializer:
> > > > >- define a constant for "windowed.inner.deserializer.class"
> > > > >- define a constant for "window.size.ms"
> > > > >
> > > > > 4) Lastly, we would update the windowed de/serializer
> implementations
> > > > > to check for the new configs (ie "
> windowed.inner.de/serializer.class
> > ")
> > > > > and use the provided de/serializer class, if one was given. If the
> > new
> > > > > configs are not present, they would fall back to the
> original/current
> > > > > logic (ie that based on the old "windowed.inner.serde.class"
> config)
> > > > >
> > > > > I think that's everything. Does this sound about right for where we
> > > want
> > > > > to go with these 

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Sophie Blee-Goldman]

A few minor notes on the KIP but otherwise I think you can go ahead and
call for a vote!

1. Need to update the Motivation section to say "console clients" or
"console consumer/producer" instead of " plain consumer client"
2. Remove the first paragraph under "Public Interfaces" (ie the KIP-writing
instructions) and also list the new config definitions here. You can just
add a code snippet for each class (TimeWindowedDe/Serializer) with the
actual variable definition you'll be adding. Maybe also add a code snippet
for StreamsConfig with the @Deprecated annotation added to the two configs
we're deprecating
3. nit: remove the "questions" under "Compatibility, Deprecation, and
Migration Plan", ie the stuff from the KIP-writing template. Just makes it
easier to read
4. In "Test Plan" we should also have a unit test to make sure the new "
windowed.inner.de/serializer.class" takes preference in the case that both
it and the old "windowed.inner.serde.class" config are both specified

Also, this is more of a question than a suggestion, but is the KIP title
perhaps a bit misleading in that people might assume these configs will no
longer be available for use at all? What do people think about changing it
to something like

*Move `window.size.ms ` and
`windowed.inner.serde.class` from `StreamsConfig` to
TimeWindowedDe/Serializer class*

A bit long/clunky but at least it gets the point across about where folks
can find these configs going forward.

On Mon, Mar 18, 2024 at 6:26 PM Lucia Cerchie 
wrote:

> Thanks for the discussion!
>
> I've updated the KIP here with what I believe are the relevant pieces,
> please let me know if anything is missing:
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=290982804
>
> On Sun, Mar 17, 2024 at 7:09 PM Sophie Blee-Goldman  >
> wrote:
>
> > Sounds good!
> >
> > @Lucia when you have a moment can you update the KIP with
> > the new proposal, including the details that Matthias pointed
> > out in his last response? After that's done I think you can go
> > ahead and call for a vote whenever you're ready!
> >
> > On Sat, Mar 16, 2024 at 7:35 PM Matthias J. Sax 
> wrote:
> >
> > > Thanks for the summary. Sounds right to me. That is what I would
> propose.
> > >
> > > As you pointed out, we of course still need to support the current
> > > confis, and we should log a warning when in use (even if the new one is
> > > in use IMHO) -- but that's more an implementation detail.
> > >
> > > I agree that the new config should take preference in case both are
> > > specified. This should be pointed out in the KIP, as it's an important
> > > contract the user needs to understand.
> > >
> > >
> > > -Matthias
> > >
> > > On 3/14/24 6:18 PM, Sophie Blee-Goldman wrote:
> > > >>
> > > >> Should we change it do `.serializer` and `.deserialize`?
> > > >
> > > > That's a good point -- if we're going to split this up by defining
> the
> > > > config
> > > > in both the TimeWindowedSerializer and TimeWindowedDeserializer,
> > > > then it makes perfect sense to go a step further and actually define
> > > > only the relevant de/serializer class instead of the full serde
> > > >
> > > > Just to put this all together, it sounds like the proposal is to:
> > > >
> > > > 1) Deprecate both these configs where they appear in StreamsConfig
> > > > (as per the original plan in the KIP, just reiterating it here)
> > > >
> > > > 2) Don't "define" either config in any specific client's Config
> class,
> > > > but just define a String variable with the config name in the
> relevant
> > > > de/serializer class, and maybe point people to it in the docs
> somewhere
> > > >
> > > > 3) We would add three new public String variables for three different
> > > > configs across two classes, specifically:
> > > >
> > > > In TimeWindowedSerializer:
> > > >- define a constant for "windowed.inner.serializer.class"
> > > > In TimeWindowedDeserializer:
> > > >- define a constant for "windowed.inner.deserializer.class"
> > > >- define a constant for "window.size.ms"
> > > >
> > > > 4) Lastly, we would update the windowed de/serializer implementations
> > > > to check for the new configs (ie "windowed.inner.de/serializer.class
> ")
> > > > and use the provided de/serializer class, if one was given. If the
> new
> > > > configs are not present, they would fall back to the original/current
> > > > logic (ie that based on the old "windowed.inner.serde.class" config)
> > > >
> > > > I think that's everything. Does this sound about right for where we
> > want
> > > > to go with these configs?
> > > >
> > > > On Thu, Mar 14, 2024 at 4:58 PM Matthias J. Sax 
> > > wrote:
> > > >
> > >  By "don't add them" do you just mean we would not have any actual
> > >  variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
> > >  and simply document -- somewhere -- that one can use the string
> > >  "window.size.ms" when configuring a command-line client with a
> > >  

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Lucia Cerchie]

Thanks for the discussion!

I've updated the KIP here with what I believe are the relevant pieces,
please let me know if anything is missing:
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=290982804

On Sun, Mar 17, 2024 at 7:09 PM Sophie Blee-Goldman 
wrote:

> Sounds good!
>
> @Lucia when you have a moment can you update the KIP with
> the new proposal, including the details that Matthias pointed
> out in his last response? After that's done I think you can go
> ahead and call for a vote whenever you're ready!
>
> On Sat, Mar 16, 2024 at 7:35 PM Matthias J. Sax  wrote:
>
> > Thanks for the summary. Sounds right to me. That is what I would propose.
> >
> > As you pointed out, we of course still need to support the current
> > confis, and we should log a warning when in use (even if the new one is
> > in use IMHO) -- but that's more an implementation detail.
> >
> > I agree that the new config should take preference in case both are
> > specified. This should be pointed out in the KIP, as it's an important
> > contract the user needs to understand.
> >
> >
> > -Matthias
> >
> > On 3/14/24 6:18 PM, Sophie Blee-Goldman wrote:
> > >>
> > >> Should we change it do `.serializer` and `.deserialize`?
> > >
> > > That's a good point -- if we're going to split this up by defining the
> > > config
> > > in both the TimeWindowedSerializer and TimeWindowedDeserializer,
> > > then it makes perfect sense to go a step further and actually define
> > > only the relevant de/serializer class instead of the full serde
> > >
> > > Just to put this all together, it sounds like the proposal is to:
> > >
> > > 1) Deprecate both these configs where they appear in StreamsConfig
> > > (as per the original plan in the KIP, just reiterating it here)
> > >
> > > 2) Don't "define" either config in any specific client's Config class,
> > > but just define a String variable with the config name in the relevant
> > > de/serializer class, and maybe point people to it in the docs somewhere
> > >
> > > 3) We would add three new public String variables for three different
> > > configs across two classes, specifically:
> > >
> > > In TimeWindowedSerializer:
> > >- define a constant for "windowed.inner.serializer.class"
> > > In TimeWindowedDeserializer:
> > >- define a constant for "windowed.inner.deserializer.class"
> > >- define a constant for "window.size.ms"
> > >
> > > 4) Lastly, we would update the windowed de/serializer implementations
> > > to check for the new configs (ie "windowed.inner.de/serializer.class")
> > > and use the provided de/serializer class, if one was given. If the new
> > > configs are not present, they would fall back to the original/current
> > > logic (ie that based on the old "windowed.inner.serde.class" config)
> > >
> > > I think that's everything. Does this sound about right for where we
> want
> > > to go with these configs?
> > >
> > > On Thu, Mar 14, 2024 at 4:58 PM Matthias J. Sax 
> > wrote:
> > >
> >  By "don't add them" do you just mean we would not have any actual
> >  variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
> >  and simply document -- somewhere -- that one can use the string
> >  "window.size.ms" when configuring a command-line client with a
> >  windowed serde?
> > >>
> > >> Yes. That's the idea.
> > >>
> > >>
> > >>
> > >>> I assume you aren't proposing
> >  to remove the ability to use and understand this config from the
> >  implementations themselves, but correct me if that's wrong.
> > >>
> > >> No, that would effectively break what we fixed with the original KIP
> :)
> > >>
> > >>
> > >>
> >  Are there any other configs in similar situations that we could look
> >  to for precedent?
> > >>
> > >> Not aware of any others, either.
> > >>
> > >>
> > >>
> >  If these are truly the first/only of their kind, I would vote to
> just
> > >> stick
> >  them in the appropriate class. As for which class to put them in, I
> >  think I'm convinced that "window.size.ms" should only go in the
> >  TimeWindowedDeserializer rather than sticking them both in the
> >  TimeWindowedSerde as I originally suggested. However, I would
> >  even go a step further and not place the "inner.window.class.serde"
> >  in the TimeWindowedSerde class either. To me, it actually makes
> >  the most sense to define it in both the TimeWindowedSerializer
> >  and the TimeWindowedDeserializer.
> > >>
> > >> Not sure either. What you propose is fine with me. However, I am
> > >> wondering about the config names... Why is it `serde` for this case?
> > >> Should we change it do `.serializer` and `.deserialize`?
> > >>
> > >>
> > >>
> > >> -Matthias
> > >>
> > >>
> > >> On 3/13/24 8:19 PM, Sophie Blee-Goldman wrote:
> > >>> By "don't add them" do you just mean we would not have any actual
> > >>> variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
> > >>> and simply document -- somewhere -- that one 

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Sophie Blee-Goldman]

Sounds good!

@Lucia when you have a moment can you update the KIP with
the new proposal, including the details that Matthias pointed
out in his last response? After that's done I think you can go
ahead and call for a vote whenever you're ready!

On Sat, Mar 16, 2024 at 7:35 PM Matthias J. Sax  wrote:

> Thanks for the summary. Sounds right to me. That is what I would propose.
>
> As you pointed out, we of course still need to support the current
> confis, and we should log a warning when in use (even if the new one is
> in use IMHO) -- but that's more an implementation detail.
>
> I agree that the new config should take preference in case both are
> specified. This should be pointed out in the KIP, as it's an important
> contract the user needs to understand.
>
>
> -Matthias
>
> On 3/14/24 6:18 PM, Sophie Blee-Goldman wrote:
> >>
> >> Should we change it do `.serializer` and `.deserialize`?
> >
> > That's a good point -- if we're going to split this up by defining the
> > config
> > in both the TimeWindowedSerializer and TimeWindowedDeserializer,
> > then it makes perfect sense to go a step further and actually define
> > only the relevant de/serializer class instead of the full serde
> >
> > Just to put this all together, it sounds like the proposal is to:
> >
> > 1) Deprecate both these configs where they appear in StreamsConfig
> > (as per the original plan in the KIP, just reiterating it here)
> >
> > 2) Don't "define" either config in any specific client's Config class,
> > but just define a String variable with the config name in the relevant
> > de/serializer class, and maybe point people to it in the docs somewhere
> >
> > 3) We would add three new public String variables for three different
> > configs across two classes, specifically:
> >
> > In TimeWindowedSerializer:
> >- define a constant for "windowed.inner.serializer.class"
> > In TimeWindowedDeserializer:
> >- define a constant for "windowed.inner.deserializer.class"
> >- define a constant for "window.size.ms"
> >
> > 4) Lastly, we would update the windowed de/serializer implementations
> > to check for the new configs (ie "windowed.inner.de/serializer.class")
> > and use the provided de/serializer class, if one was given. If the new
> > configs are not present, they would fall back to the original/current
> > logic (ie that based on the old "windowed.inner.serde.class" config)
> >
> > I think that's everything. Does this sound about right for where we want
> > to go with these configs?
> >
> > On Thu, Mar 14, 2024 at 4:58 PM Matthias J. Sax 
> wrote:
> >
>  By "don't add them" do you just mean we would not have any actual
>  variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
>  and simply document -- somewhere -- that one can use the string
>  "window.size.ms" when configuring a command-line client with a
>  windowed serde?
> >>
> >> Yes. That's the idea.
> >>
> >>
> >>
> >>> I assume you aren't proposing
>  to remove the ability to use and understand this config from the
>  implementations themselves, but correct me if that's wrong.
> >>
> >> No, that would effectively break what we fixed with the original KIP :)
> >>
> >>
> >>
>  Are there any other configs in similar situations that we could look
>  to for precedent?
> >>
> >> Not aware of any others, either.
> >>
> >>
> >>
>  If these are truly the first/only of their kind, I would vote to just
> >> stick
>  them in the appropriate class. As for which class to put them in, I
>  think I'm convinced that "window.size.ms" should only go in the
>  TimeWindowedDeserializer rather than sticking them both in the
>  TimeWindowedSerde as I originally suggested. However, I would
>  even go a step further and not place the "inner.window.class.serde"
>  in the TimeWindowedSerde class either. To me, it actually makes
>  the most sense to define it in both the TimeWindowedSerializer
>  and the TimeWindowedDeserializer.
> >>
> >> Not sure either. What you propose is fine with me. However, I am
> >> wondering about the config names... Why is it `serde` for this case?
> >> Should we change it do `.serializer` and `.deserialize`?
> >>
> >>
> >>
> >> -Matthias
> >>
> >>
> >> On 3/13/24 8:19 PM, Sophie Blee-Goldman wrote:
> >>> By "don't add them" do you just mean we would not have any actual
> >>> variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
> >>> and simply document -- somewhere -- that one can use the string
> >>> "window.size.ms" when configuring a command-line client with a
> >>> windowed serde? Or something else? I assume you aren't proposing
> >>> to remove the ability to use and understand this config from the
> >>> implementations themselves, but correct me if that's wrong.
> >>>
> >>> Are there any other configs in similar situations that we could look
> >>> to for precedent? I personally am not aware of any but by definition
> >>> I suppose these would be hard to discover 

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Matthias J. Sax]

Thanks for the summary. Sounds right to me. That is what I would propose.

As you pointed out, we of course still need to support the current 
confis, and we should log a warning when in use (even if the new one is 
in use IMHO) -- but that's more an implementation detail.


I agree that the new config should take preference in case both are 
specified. This should be pointed out in the KIP, as it's an important 
contract the user needs to understand.



-Matthias

On 3/14/24 6:18 PM, Sophie Blee-Goldman wrote:


Should we change it do `.serializer` and `.deserialize`?


That's a good point -- if we're going to split this up by defining the
config
in both the TimeWindowedSerializer and TimeWindowedDeserializer,
then it makes perfect sense to go a step further and actually define
only the relevant de/serializer class instead of the full serde

Just to put this all together, it sounds like the proposal is to:

1) Deprecate both these configs where they appear in StreamsConfig
(as per the original plan in the KIP, just reiterating it here)

2) Don't "define" either config in any specific client's Config class,
but just define a String variable with the config name in the relevant
de/serializer class, and maybe point people to it in the docs somewhere

3) We would add three new public String variables for three different
configs across two classes, specifically:

In TimeWindowedSerializer:
   - define a constant for "windowed.inner.serializer.class"
In TimeWindowedDeserializer:
   - define a constant for "windowed.inner.deserializer.class"
   - define a constant for "window.size.ms"

4) Lastly, we would update the windowed de/serializer implementations
to check for the new configs (ie "windowed.inner.de/serializer.class")
and use the provided de/serializer class, if one was given. If the new
configs are not present, they would fall back to the original/current
logic (ie that based on the old "windowed.inner.serde.class" config)

I think that's everything. Does this sound about right for where we want
to go with these configs?

On Thu, Mar 14, 2024 at 4:58 PM Matthias J. Sax  wrote:


By "don't add them" do you just mean we would not have any actual
variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
and simply document -- somewhere -- that one can use the string
"window.size.ms" when configuring a command-line client with a
windowed serde?


Yes. That's the idea.




I assume you aren't proposing

to remove the ability to use and understand this config from the
implementations themselves, but correct me if that's wrong.


No, that would effectively break what we fixed with the original KIP :)




Are there any other configs in similar situations that we could look
to for precedent?


Not aware of any others, either.




If these are truly the first/only of their kind, I would vote to just

stick

them in the appropriate class. As for which class to put them in, I
think I'm convinced that "window.size.ms" should only go in the
TimeWindowedDeserializer rather than sticking them both in the
TimeWindowedSerde as I originally suggested. However, I would
even go a step further and not place the "inner.window.class.serde"
in the TimeWindowedSerde class either. To me, it actually makes
the most sense to define it in both the TimeWindowedSerializer
and the TimeWindowedDeserializer.


Not sure either. What you propose is fine with me. However, I am
wondering about the config names... Why is it `serde` for this case?
Should we change it do `.serializer` and `.deserialize`?



-Matthias


On 3/13/24 8:19 PM, Sophie Blee-Goldman wrote:

By "don't add them" do you just mean we would not have any actual
variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
and simply document -- somewhere -- that one can use the string
"window.size.ms" when configuring a command-line client with a
windowed serde? Or something else? I assume you aren't proposing
to remove the ability to use and understand this config from the
implementations themselves, but correct me if that's wrong.

Are there any other configs in similar situations that we could look
to for precedent? I personally am not aware of any but by definition
I suppose these would be hard to discover unless you were actively
looking for them, so I'm wondering if there might be other "shadow
configs" elsewhere in the code base.

If these are truly the first/only of their kind, I would vote to just

stick

them in the appropriate class. As for which class to put them in, I
think I'm convinced that "window.size.ms" should only go in the
TimeWindowedDeserializer rather than sticking them both in the
TimeWindowedSerde as I originally suggested. However, I would
even go a step further and not place the "inner.window.class.serde"
in the TimeWindowedSerde class either. To me, it actually makes
the most sense to define it in both the TimeWindowedSerializer
and the TimeWindowedDeserializer.

The reason being that, as discussed above, the only use case for
these 

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Sophie Blee-Goldman]

>
> Should we change it do `.serializer` and `.deserialize`?

That's a good point -- if we're going to split this up by defining the
config
in both the TimeWindowedSerializer and TimeWindowedDeserializer,
then it makes perfect sense to go a step further and actually define
only the relevant de/serializer class instead of the full serde

Just to put this all together, it sounds like the proposal is to:

1) Deprecate both these configs where they appear in StreamsConfig
(as per the original plan in the KIP, just reiterating it here)

2) Don't "define" either config in any specific client's Config class,
but just define a String variable with the config name in the relevant
de/serializer class, and maybe point people to it in the docs somewhere

3) We would add three new public String variables for three different
configs across two classes, specifically:

In TimeWindowedSerializer:
  - define a constant for "windowed.inner.serializer.class"
In TimeWindowedDeserializer:
  - define a constant for "windowed.inner.deserializer.class"
  - define a constant for "window.size.ms"

4) Lastly, we would update the windowed de/serializer implementations
to check for the new configs (ie "windowed.inner.de/serializer.class")
and use the provided de/serializer class, if one was given. If the new
configs are not present, they would fall back to the original/current
logic (ie that based on the old "windowed.inner.serde.class" config)

I think that's everything. Does this sound about right for where we want
to go with these configs?

On Thu, Mar 14, 2024 at 4:58 PM Matthias J. Sax  wrote:

> >> By "don't add them" do you just mean we would not have any actual
> >> variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
> >> and simply document -- somewhere -- that one can use the string
> >> "window.size.ms" when configuring a command-line client with a
> >> windowed serde?
>
> Yes. That's the idea.
>
>
>
> > I assume you aren't proposing
> >> to remove the ability to use and understand this config from the
> >> implementations themselves, but correct me if that's wrong.
>
> No, that would effectively break what we fixed with the original KIP :)
>
>
>
> >> Are there any other configs in similar situations that we could look
> >> to for precedent?
>
> Not aware of any others, either.
>
>
>
> >> If these are truly the first/only of their kind, I would vote to just
> stick
> >> them in the appropriate class. As for which class to put them in, I
> >> think I'm convinced that "window.size.ms" should only go in the
> >> TimeWindowedDeserializer rather than sticking them both in the
> >> TimeWindowedSerde as I originally suggested. However, I would
> >> even go a step further and not place the "inner.window.class.serde"
> >> in the TimeWindowedSerde class either. To me, it actually makes
> >> the most sense to define it in both the TimeWindowedSerializer
> >> and the TimeWindowedDeserializer.
>
> Not sure either. What you propose is fine with me. However, I am
> wondering about the config names... Why is it `serde` for this case?
> Should we change it do `.serializer` and `.deserialize`?
>
>
>
> -Matthias
>
>
> On 3/13/24 8:19 PM, Sophie Blee-Goldman wrote:
> > By "don't add them" do you just mean we would not have any actual
> > variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
> > and simply document -- somewhere -- that one can use the string
> > "window.size.ms" when configuring a command-line client with a
> > windowed serde? Or something else? I assume you aren't proposing
> > to remove the ability to use and understand this config from the
> > implementations themselves, but correct me if that's wrong.
> >
> > Are there any other configs in similar situations that we could look
> > to for precedent? I personally am not aware of any but by definition
> > I suppose these would be hard to discover unless you were actively
> > looking for them, so I'm wondering if there might be other "shadow
> > configs" elsewhere in the code base.
> >
> > If these are truly the first/only of their kind, I would vote to just
> stick
> > them in the appropriate class. As for which class to put them in, I
> > think I'm convinced that "window.size.ms" should only go in the
> > TimeWindowedDeserializer rather than sticking them both in the
> > TimeWindowedSerde as I originally suggested. However, I would
> > even go a step further and not place the "inner.window.class.serde"
> > in the TimeWindowedSerde class either. To me, it actually makes
> > the most sense to define it in both the TimeWindowedSerializer
> > and the TimeWindowedDeserializer.
> >
> > The reason being that, as discussed above, the only use case for
> > these configs would be in the console consumer/producer which
> > only uses the Serializer or Deserializer, and would never actually
> > be used by/in Streams where we use the Serde version. And while
> > defining the  "inner.window.class.serde" in two places might seem
> > redundant, this would mean that all 

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Matthias J. Sax]

By "don't add them" do you just mean we would not have any actual
variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
and simply document -- somewhere -- that one can use the string
"window.size.ms" when configuring a command-line client with a
windowed serde? 


Yes. That's the idea.




I assume you aren't proposing

to remove the ability to use and understand this config from the
implementations themselves, but correct me if that's wrong.


No, that would effectively break what we fixed with the original KIP :)




Are there any other configs in similar situations that we could look
to for precedent?


Not aware of any others, either.




If these are truly the first/only of their kind, I would vote to just stick
them in the appropriate class. As for which class to put them in, I
think I'm convinced that "window.size.ms" should only go in the
TimeWindowedDeserializer rather than sticking them both in the
TimeWindowedSerde as I originally suggested. However, I would
even go a step further and not place the "inner.window.class.serde"
in the TimeWindowedSerde class either. To me, it actually makes
the most sense to define it in both the TimeWindowedSerializer
and the TimeWindowedDeserializer.


Not sure either. What you propose is fine with me. However, I am 
wondering about the config names... Why is it `serde` for this case? 
Should we change it do `.serializer` and `.deserialize`?




-Matthias


On 3/13/24 8:19 PM, Sophie Blee-Goldman wrote:

By "don't add them" do you just mean we would not have any actual
variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
and simply document -- somewhere -- that one can use the string
"window.size.ms" when configuring a command-line client with a
windowed serde? Or something else? I assume you aren't proposing
to remove the ability to use and understand this config from the
implementations themselves, but correct me if that's wrong.

Are there any other configs in similar situations that we could look
to for precedent? I personally am not aware of any but by definition
I suppose these would be hard to discover unless you were actively
looking for them, so I'm wondering if there might be other "shadow
configs" elsewhere in the code base.

If these are truly the first/only of their kind, I would vote to just stick
them in the appropriate class. As for which class to put them in, I
think I'm convinced that "window.size.ms" should only go in the
TimeWindowedDeserializer rather than sticking them both in the
TimeWindowedSerde as I originally suggested. However, I would
even go a step further and not place the "inner.window.class.serde"
in the TimeWindowedSerde class either. To me, it actually makes
the most sense to define it in both the TimeWindowedSerializer
and the TimeWindowedDeserializer.

The reason being that, as discussed above, the only use case for
these configs would be in the console consumer/producer which
only uses the Serializer or Deserializer, and would never actually
be used by/in Streams where we use the Serde version. And while
defining the  "inner.window.class.serde" in two places might seem
redundant, this would mean that all the configs needed to properly
configure the specific class being used by the particular kind of
consumer client -- that is, Deserializer for a console consumer and
Serializer for a console producer -- would be located in that exact
class. I assume this would make them much easier to discover
and be used than having to search for configs defined in classes
you don't even need for the console client, like the Serde form

Just my two cents -- happy to hear other opinions on this

On Mon, Mar 11, 2024 at 6:58 PM Matthias J. Sax  wrote:


Yes, it's used inside `TimeWindowedSerializer` and actually also inside
`TimeWindowDeserializer`.

However, it does IMHO not change that we should remove it from
`StreamsConfig` because both configs are not intended to be used in Java
code... If one writes Java code, they should use

new TimeWindowedSerializer(Serializer)
new TimeWindowDeserializer(Deserializer, Long)
new TimeWindowSerdes(Serde, Long)

and thus they don't need either config.

The configs are only needed for command line tool, that create the
(de)serializer via reflection using the default constructor.

Does this make sense?



The only open question is really, if and where to add them... Strictly
speaking, we don't need either config as public variable as nobody
should use them in Java code. To me, it just feels right/better do make
them public for documentation purpose that these configs exists?

`inner.window.class.serde` has "serde" in it's name, so we could add it
to `TimeWindowSerdes`? For `window.size.ms`, it's only used by the
deserialize to maybe add it there? Just some ideas. -- Or we sidestep
this question and just don't add them; also fine with me.


-Matthias

On 3/11/24 10:53 AM, Lucia Cerchie wrote:

PS-- I was re-reading the PR that originated this discussion and realized
that 

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Sophie Blee-Goldman]

By "don't add them" do you just mean we would not have any actual
variables defined anywhere for these configs (eg WINDOW_SIZE_MS)
and simply document -- somewhere -- that one can use the string
"window.size.ms" when configuring a command-line client with a
windowed serde? Or something else? I assume you aren't proposing
to remove the ability to use and understand this config from the
implementations themselves, but correct me if that's wrong.

Are there any other configs in similar situations that we could look
to for precedent? I personally am not aware of any but by definition
I suppose these would be hard to discover unless you were actively
looking for them, so I'm wondering if there might be other "shadow
configs" elsewhere in the code base.

If these are truly the first/only of their kind, I would vote to just stick
them in the appropriate class. As for which class to put them in, I
think I'm convinced that "window.size.ms" should only go in the
TimeWindowedDeserializer rather than sticking them both in the
TimeWindowedSerde as I originally suggested. However, I would
even go a step further and not place the "inner.window.class.serde"
in the TimeWindowedSerde class either. To me, it actually makes
the most sense to define it in both the TimeWindowedSerializer
and the TimeWindowedDeserializer.

The reason being that, as discussed above, the only use case for
these configs would be in the console consumer/producer which
only uses the Serializer or Deserializer, and would never actually
be used by/in Streams where we use the Serde version. And while
defining the  "inner.window.class.serde" in two places might seem
redundant, this would mean that all the configs needed to properly
configure the specific class being used by the particular kind of
consumer client -- that is, Deserializer for a console consumer and
Serializer for a console producer -- would be located in that exact
class. I assume this would make them much easier to discover
and be used than having to search for configs defined in classes
you don't even need for the console client, like the Serde form

Just my two cents -- happy to hear other opinions on this

On Mon, Mar 11, 2024 at 6:58 PM Matthias J. Sax  wrote:

> Yes, it's used inside `TimeWindowedSerializer` and actually also inside
> `TimeWindowDeserializer`.
>
> However, it does IMHO not change that we should remove it from
> `StreamsConfig` because both configs are not intended to be used in Java
> code... If one writes Java code, they should use
>
>new TimeWindowedSerializer(Serializer)
>new TimeWindowDeserializer(Deserializer, Long)
>new TimeWindowSerdes(Serde, Long)
>
> and thus they don't need either config.
>
> The configs are only needed for command line tool, that create the
> (de)serializer via reflection using the default constructor.
>
> Does this make sense?
>
>
>
> The only open question is really, if and where to add them... Strictly
> speaking, we don't need either config as public variable as nobody
> should use them in Java code. To me, it just feels right/better do make
> them public for documentation purpose that these configs exists?
>
> `inner.window.class.serde` has "serde" in it's name, so we could add it
> to `TimeWindowSerdes`? For `window.size.ms`, it's only used by the
> deserialize to maybe add it there? Just some ideas. -- Or we sidestep
> this question and just don't add them; also fine with me.
>
>
> -Matthias
>
> On 3/11/24 10:53 AM, Lucia Cerchie wrote:
> > PS-- I was re-reading the PR that originated this discussion and realized
> > that `window.inner.serde.class` is used here
> > <
> https://github.com/a0x8o/kafka/blob/master/streams/src/main/java/org/apache/kafka/streams/kstream/TimeWindowedSerializer.java#L44
> >
> > in KStreams. This goes against removing it, yes?
> >
> > On Mon, Mar 11, 2024 at 10:40 AM Lucia Cerchie 
> > wrote:
> >
> >> Sophie, I'll add a paragraph about removing `windowed.inner.serde.class`
> >> to the KIP. I'll also add putting it in the `TimeWindowedSerde` class
> with
> >> some add'tl guidance on the docs addition.
> >>
> >> Also, I double-checked setting window.size.ms on the client and it
> >> doesn't throw an error at all, in response to Matthias's question.
> Changing
> >> the KIP in response to that.
> >>
> >> On Sun, Mar 10, 2024 at 6:04 PM Sophie Blee-Goldman <
> sop...@responsive.dev>
> >> wrote:
> >>
> >>> Thanks for responding Matthias -- you got there first, but I was going
> to
> >>> say exactly the same thing as in your most reply. In other words, I see
> >>> the
> >>> `windowed.inner.serde.class` as being in the same boat as the `
> >>> window.size.ms` config, so whatever we do with one we should do for
> the
> >>> other.
> >>>
> >>> I do agree with removing these from StreamsConfig, but defining them in
> >>> ConsumerConfig feels weird as well. There's really no great answer
> here.
> >>>
> >>> My only concern about adding it to the corresponding
> >>> serde/serializer/deserializer class is 

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Matthias J. Sax]

Yes, it's used inside `TimeWindowedSerializer` and actually also inside 
`TimeWindowDeserializer`.


However, it does IMHO not change that we should remove it from 
`StreamsConfig` because both configs are not intended to be used in Java 
code... If one writes Java code, they should use


  new TimeWindowedSerializer(Serializer)
  new TimeWindowDeserializer(Deserializer, Long)
  new TimeWindowSerdes(Serde, Long)

and thus they don't need either config.

The configs are only needed for command line tool, that create the 
(de)serializer via reflection using the default constructor.


Does this make sense?



The only open question is really, if and where to add them... Strictly 
speaking, we don't need either config as public variable as nobody 
should use them in Java code. To me, it just feels right/better do make 
them public for documentation purpose that these configs exists?


`inner.window.class.serde` has "serde" in it's name, so we could add it 
to `TimeWindowSerdes`? For `window.size.ms`, it's only used by the 
deserialize to maybe add it there? Just some ideas. -- Or we sidestep 
this question and just don't add them; also fine with me.



-Matthias

On 3/11/24 10:53 AM, Lucia Cerchie wrote:

PS-- I was re-reading the PR that originated this discussion and realized
that `window.inner.serde.class` is used here

in KStreams. This goes against removing it, yes?

On Mon, Mar 11, 2024 at 10:40 AM Lucia Cerchie 
wrote:


Sophie, I'll add a paragraph about removing `windowed.inner.serde.class`
to the KIP. I'll also add putting it in the `TimeWindowedSerde` class with
some add'tl guidance on the docs addition.

Also, I double-checked setting window.size.ms on the client and it
doesn't throw an error at all, in response to Matthias's question. Changing
the KIP in response to that.

On Sun, Mar 10, 2024 at 6:04 PM Sophie Blee-Goldman 
wrote:


Thanks for responding Matthias -- you got there first, but I was going to
say exactly the same thing as in your most reply. In other words, I see
the
`windowed.inner.serde.class` as being in the same boat as the `
window.size.ms` config, so whatever we do with one we should do for the
other.

I do agree with removing these from StreamsConfig, but defining them in
ConsumerConfig feels weird as well. There's really no great answer here.

My only concern about adding it to the corresponding
serde/serializer/deserializer class is that it might be difficult for
people to find them. I generally assume that people tend not to look at
the
serde/serializer/deserializer classes/implementations. But maybe in this
case, someone who actually needed these configs would be likely to be
motivated enough to find them by looking at the class? And with sufficient
documentation, it's likely not a problem. So, I'm +1 on putting it into
the
TimeWindowedSerde class

(I would personally stick them into the serde class, rather than the
serializer and/or deserializer, but I could be convinced either way)

On Fri, Mar 1, 2024 at 3:00 PM Matthias J. Sax  wrote:


One more thought after I did some more digging on the related PR.

Should we do the same thing for `windowed.inner.serde.class`?


Both config belong to windowed serdes (which KS provide) but the KS code
itself does never use them (and in fact, disallows to use them and would
throw an error is used). Both are intended for plain consumer use cases
for which the window serdes are used.

The question to me is, should we add them back somewhere else? It does
not really belong into `ConsumerConfig` either, but maybe we could add
them to the corresponding serde or (de)serialize classes?


-Matthias


On 2/21/24 2:41 PM, Matthias J. Sax wrote:

Thanks for the KIP. Sounds like a nice cleanup.


window.size.ms  is not a true KafkaStreams config, and results in an
error when set from a KStreams application


What error?


Given that the configs is used by `TimeWindowedDeserializer` I am
wondering if we should additionally add

public class TimeWindowedDeserializer {

  public static final String WINDOW_SIZE_MS_CONFIG = "

window.size.ms

";

}



-Matthias


On 2/15/24 6:32 AM, Lucia Cerchie wrote:

Hey everyone,

I'd like to discuss KIP-1020
<



https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=290982804

,

which would move to deprecate `window.size.ms` in `StreamsConfig`

since `

window.size.ms` is a client config.

Thanks in advance!

Lucia Cerchie








--

[image: Confluent] 
Lucia Cerchie
Developer Advocate
Follow us: [image: Blog]
[image:
Twitter] [image: Slack]
[image: YouTube]


[image: Try Confluent Cloud for Free]

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Lucia Cerchie]

PS-- I was re-reading the PR that originated this discussion and realized
that `window.inner.serde.class` is used here

in KStreams. This goes against removing it, yes?

On Mon, Mar 11, 2024 at 10:40 AM Lucia Cerchie 
wrote:

> Sophie, I'll add a paragraph about removing `windowed.inner.serde.class`
> to the KIP. I'll also add putting it in the `TimeWindowedSerde` class with
> some add'tl guidance on the docs addition.
>
> Also, I double-checked setting window.size.ms on the client and it
> doesn't throw an error at all, in response to Matthias's question. Changing
> the KIP in response to that.
>
> On Sun, Mar 10, 2024 at 6:04 PM Sophie Blee-Goldman 
> wrote:
>
>> Thanks for responding Matthias -- you got there first, but I was going to
>> say exactly the same thing as in your most reply. In other words, I see
>> the
>> `windowed.inner.serde.class` as being in the same boat as the `
>> window.size.ms` config, so whatever we do with one we should do for the
>> other.
>>
>> I do agree with removing these from StreamsConfig, but defining them in
>> ConsumerConfig feels weird as well. There's really no great answer here.
>>
>> My only concern about adding it to the corresponding
>> serde/serializer/deserializer class is that it might be difficult for
>> people to find them. I generally assume that people tend not to look at
>> the
>> serde/serializer/deserializer classes/implementations. But maybe in this
>> case, someone who actually needed these configs would be likely to be
>> motivated enough to find them by looking at the class? And with sufficient
>> documentation, it's likely not a problem. So, I'm +1 on putting it into
>> the
>> TimeWindowedSerde class
>>
>> (I would personally stick them into the serde class, rather than the
>> serializer and/or deserializer, but I could be convinced either way)
>>
>> On Fri, Mar 1, 2024 at 3:00 PM Matthias J. Sax  wrote:
>>
>> > One more thought after I did some more digging on the related PR.
>> >
>> > Should we do the same thing for `windowed.inner.serde.class`?
>> >
>> >
>> > Both config belong to windowed serdes (which KS provide) but the KS code
>> > itself does never use them (and in fact, disallows to use them and would
>> > throw an error is used). Both are intended for plain consumer use cases
>> > for which the window serdes are used.
>> >
>> > The question to me is, should we add them back somewhere else? It does
>> > not really belong into `ConsumerConfig` either, but maybe we could add
>> > them to the corresponding serde or (de)serialize classes?
>> >
>> >
>> > -Matthias
>> >
>> >
>> > On 2/21/24 2:41 PM, Matthias J. Sax wrote:
>> > > Thanks for the KIP. Sounds like a nice cleanup.
>> > >
>> > >> window.size.ms  is not a true KafkaStreams config, and results in an
>> > >> error when set from a KStreams application
>> > >
>> > > What error?
>> > >
>> > >
>> > > Given that the configs is used by `TimeWindowedDeserializer` I am
>> > > wondering if we should additionally add
>> > >
>> > > public class TimeWindowedDeserializer {
>> > >
>> > >  public static final String WINDOW_SIZE_MS_CONFIG = "
>> window.size.ms
>> > ";
>> > > }
>> > >
>> > >
>> > >
>> > > -Matthias
>> > >
>> > >
>> > > On 2/15/24 6:32 AM, Lucia Cerchie wrote:
>> > >> Hey everyone,
>> > >>
>> > >> I'd like to discuss KIP-1020
>> > >> <
>> >
>> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=290982804
>> > >,
>> > >> which would move to deprecate `window.size.ms` in `StreamsConfig`
>> > since `
>> > >> window.size.ms` is a client config.
>> > >>
>> > >> Thanks in advance!
>> > >>
>> > >> Lucia Cerchie
>> > >>
>> >
>>
>
>
> --
>
> [image: Confluent] 
> Lucia Cerchie
> Developer Advocate
> Follow us: [image: Blog]
> [image:
> Twitter] [image: Slack]
> [image: YouTube]
> 
>
> [image: Try Confluent Cloud for Free]
> 
>


-- 

[image: Confluent] 
Lucia Cerchie
Developer Advocate
Follow us: [image: Blog]
[image:
Twitter] [image: Slack]
[image: YouTube]


[image: Try Confluent Cloud for Free]

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Lucia Cerchie]

Sophie, I'll add a paragraph about removing `windowed.inner.serde.class` to
the KIP. I'll also add putting it in the `TimeWindowedSerde` class with
some add'tl guidance on the docs addition.

Also, I double-checked setting window.size.ms on the client and it doesn't
throw an error at all, in response to Matthias's question. Changing the KIP
in response to that.

On Sun, Mar 10, 2024 at 6:04 PM Sophie Blee-Goldman 
wrote:

> Thanks for responding Matthias -- you got there first, but I was going to
> say exactly the same thing as in your most reply. In other words, I see the
> `windowed.inner.serde.class` as being in the same boat as the `
> window.size.ms` config, so whatever we do with one we should do for the
> other.
>
> I do agree with removing these from StreamsConfig, but defining them in
> ConsumerConfig feels weird as well. There's really no great answer here.
>
> My only concern about adding it to the corresponding
> serde/serializer/deserializer class is that it might be difficult for
> people to find them. I generally assume that people tend not to look at the
> serde/serializer/deserializer classes/implementations. But maybe in this
> case, someone who actually needed these configs would be likely to be
> motivated enough to find them by looking at the class? And with sufficient
> documentation, it's likely not a problem. So, I'm +1 on putting it into the
> TimeWindowedSerde class
>
> (I would personally stick them into the serde class, rather than the
> serializer and/or deserializer, but I could be convinced either way)
>
> On Fri, Mar 1, 2024 at 3:00 PM Matthias J. Sax  wrote:
>
> > One more thought after I did some more digging on the related PR.
> >
> > Should we do the same thing for `windowed.inner.serde.class`?
> >
> >
> > Both config belong to windowed serdes (which KS provide) but the KS code
> > itself does never use them (and in fact, disallows to use them and would
> > throw an error is used). Both are intended for plain consumer use cases
> > for which the window serdes are used.
> >
> > The question to me is, should we add them back somewhere else? It does
> > not really belong into `ConsumerConfig` either, but maybe we could add
> > them to the corresponding serde or (de)serialize classes?
> >
> >
> > -Matthias
> >
> >
> > On 2/21/24 2:41 PM, Matthias J. Sax wrote:
> > > Thanks for the KIP. Sounds like a nice cleanup.
> > >
> > >> window.size.ms  is not a true KafkaStreams config, and results in an
> > >> error when set from a KStreams application
> > >
> > > What error?
> > >
> > >
> > > Given that the configs is used by `TimeWindowedDeserializer` I am
> > > wondering if we should additionally add
> > >
> > > public class TimeWindowedDeserializer {
> > >
> > >  public static final String WINDOW_SIZE_MS_CONFIG = "
> window.size.ms
> > ";
> > > }
> > >
> > >
> > >
> > > -Matthias
> > >
> > >
> > > On 2/15/24 6:32 AM, Lucia Cerchie wrote:
> > >> Hey everyone,
> > >>
> > >> I'd like to discuss KIP-1020
> > >> <
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=290982804
> > >,
> > >> which would move to deprecate `window.size.ms` in `StreamsConfig`
> > since `
> > >> window.size.ms` is a client config.
> > >>
> > >> Thanks in advance!
> > >>
> > >> Lucia Cerchie
> > >>
> >
>


-- 

[image: Confluent] 
Lucia Cerchie
Developer Advocate
Follow us: [image: Blog]
[image:
Twitter] [image: Slack]
[image: YouTube]


[image: Try Confluent Cloud for Free]

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Sophie Blee-Goldman]

Thanks for responding Matthias -- you got there first, but I was going to
say exactly the same thing as in your most reply. In other words, I see the
`windowed.inner.serde.class` as being in the same boat as the `
window.size.ms` config, so whatever we do with one we should do for the
other.

I do agree with removing these from StreamsConfig, but defining them in
ConsumerConfig feels weird as well. There's really no great answer here.

My only concern about adding it to the corresponding
serde/serializer/deserializer class is that it might be difficult for
people to find them. I generally assume that people tend not to look at the
serde/serializer/deserializer classes/implementations. But maybe in this
case, someone who actually needed these configs would be likely to be
motivated enough to find them by looking at the class? And with sufficient
documentation, it's likely not a problem. So, I'm +1 on putting it into the
TimeWindowedSerde class

(I would personally stick them into the serde class, rather than the
serializer and/or deserializer, but I could be convinced either way)

On Fri, Mar 1, 2024 at 3:00 PM Matthias J. Sax  wrote:

> One more thought after I did some more digging on the related PR.
>
> Should we do the same thing for `windowed.inner.serde.class`?
>
>
> Both config belong to windowed serdes (which KS provide) but the KS code
> itself does never use them (and in fact, disallows to use them and would
> throw an error is used). Both are intended for plain consumer use cases
> for which the window serdes are used.
>
> The question to me is, should we add them back somewhere else? It does
> not really belong into `ConsumerConfig` either, but maybe we could add
> them to the corresponding serde or (de)serialize classes?
>
>
> -Matthias
>
>
> On 2/21/24 2:41 PM, Matthias J. Sax wrote:
> > Thanks for the KIP. Sounds like a nice cleanup.
> >
> >> window.size.ms  is not a true KafkaStreams config, and results in an
> >> error when set from a KStreams application
> >
> > What error?
> >
> >
> > Given that the configs is used by `TimeWindowedDeserializer` I am
> > wondering if we should additionally add
> >
> > public class TimeWindowedDeserializer {
> >
> >  public static final String WINDOW_SIZE_MS_CONFIG = "window.size.ms
> ";
> > }
> >
> >
> >
> > -Matthias
> >
> >
> > On 2/15/24 6:32 AM, Lucia Cerchie wrote:
> >> Hey everyone,
> >>
> >> I'd like to discuss KIP-1020
> >> <
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=290982804
> >,
> >> which would move to deprecate `window.size.ms` in `StreamsConfig`
> since `
> >> window.size.ms` is a client config.
> >>
> >> Thanks in advance!
> >>
> >> Lucia Cerchie
> >>
>

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Matthias J. Sax]

One more thought after I did some more digging on the related PR.

Should we do the same thing for `windowed.inner.serde.class`?


Both config belong to windowed serdes (which KS provide) but the KS code 
itself does never use them (and in fact, disallows to use them and would 
throw an error is used). Both are intended for plain consumer use cases 
for which the window serdes are used.


The question to me is, should we add them back somewhere else? It does 
not really belong into `ConsumerConfig` either, but maybe we could add 
them to the corresponding serde or (de)serialize classes?



-Matthias


On 2/21/24 2:41 PM, Matthias J. Sax wrote:

Thanks for the KIP. Sounds like a nice cleanup.

window.size.ms  is not a true KafkaStreams config, and results in an 
error when set from a KStreams application


What error?


Given that the configs is used by `TimeWindowedDeserializer` I am 
wondering if we should additionally add


public class TimeWindowedDeserializer {

     public static final String WINDOW_SIZE_MS_CONFIG = "window.size.ms";
}



-Matthias


On 2/15/24 6:32 AM, Lucia Cerchie wrote:

Hey everyone,

I'd like to discuss KIP-1020
,
which would move to deprecate `window.size.ms` in `StreamsConfig` since `
window.size.ms` is a client config.

Thanks in advance!

Lucia Cerchie

Re: [DISCUSS] KIP-1020: Deprecate `window.size.ms` in `StreamsConfig`

[Matthias J. Sax]

Thanks for the KIP. Sounds like a nice cleanup.


window.size.ms  is not a true KafkaStreams config, and results in an error when 
set from a KStreams application


What error?


Given that the configs is used by `TimeWindowedDeserializer` I am 
wondering if we should additionally add


public class TimeWindowedDeserializer {

public static final String WINDOW_SIZE_MS_CONFIG = "window.size.ms";
}



-Matthias


On 2/15/24 6:32 AM, Lucia Cerchie wrote:

Hey everyone,

I'd like to discuss KIP-1020
,
which would move to deprecate `window.size.ms` in `StreamsConfig` since `
window.size.ms` is a client config.

Thanks in advance!

Lucia Cerchie