You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@kafka.apache.org by Sagar <sa...@gmail.com> on 2021/03/22 10:28:08 UTC

[DISCUSS] KIP-725: Streamlining configurations for TimeWindowedDeserializer.

Hi All,

I would like to start a discussion on the following KIP:

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

Thanks!
Sagar.

Re: [DISCUSS] KIP-725: Streamlining configurations for TimeWindowedDeserializer.

Posted by Sophie Blee-Goldman <so...@confluent.io.INVALID>.

Thanks Sagar! The KIP looks good to me now. Let's see what others think

On Fri, Mar 26, 2021 at 1:36 AM Sagar <sa...@gmail.com> wrote:

> Hi Sophie,
>
> Thanks for the feedback! I have updated the KIP inline with whatever you
> suggested.
>
> Regarding point 5, I have added the note as it makes sense to not set the
> config via the KafkaStreams app.
>
> Thanks!
> Sagar.
>
>
> On Wed, Mar 24, 2021 at 7:52 AM Sophie Blee-Goldman
> <so...@confluent.io.invalid> wrote:
>
> > Hey Sagar,
> >
> > Thanks for the KIP! The overall proposal looks good to me, but I had some
> > miscellaneous notes:
> >
> > 1) Some general KIP-writing advice:
> >     - You don't need to list any implementation details, only public
> > interfaces that are being added, modified, or
> >       deprecated. It's better to describe your changes in words, or
> > occasionally psuedo-code for more complicated
> >       changes or algorithms. The KIP is a public contract, so you
> generally
> > want to agree upon the high-level
> >       proposal and avoid getting locked in to specific code which you may
> > end up wanting to change once you
> >       start on the PR.
> >       In this KIP, you can remove the code block under
> > *TimeWindowedDeserializer
> > *and just describe the desired
> >       semantics: eg that you should only use the constructor within
> > Streams, the configs within the console consumer,
> >       or either for a plain consumer client provided they match.
> >       The code block under *StreamsConfig *however is a good example of
> > what should be in a KIP. Only one nit:
> >       in the doc string, avoid saying "windowed key" and just say
> "windowed
> > record" or something like that.
> >     - The *Motivation* section should be focused on any background or
> > additional context that's necessary to
> >       understand the KIP, as well as the actual motivation for the change
> > being proposed. So here, everything under
> >       the second bullet which begins with "The KIP also introduces
> > changes..." should be cut from that section and
> >       discussed elsewhere.
> >     - The *Proposed Changes* and *Public Interfaces* sections have a lot
> of
> > overlap and repeated content. To be
> >       honest I personally have struggled with deciding which section to
> > include something in, but a good rule of thumb
> >       is to describe the actual changes in the *Proposed Changes*
> section,
> > and then use the *Public Interfaces* section
> >       to list any new or modified public APIs. In this case, I would move
> > everything to the *Proposed Changes* section
> >       except for the code block with the deprecated config(s) and the new
> > config + doc string.
> >
> > 2) Can you make it clear that both *default.windowed.key.serde.inner* and
> > *default.windowed.key.serde.inner *
> > are being deprecated, and we're adding a new config called
> > *windowed.deserializer.inner.class*, not literally
> > renaming the existing *default.windowed.key.serde.inner* config? I think
> > you're hinting at this in the
> > *Compatibility, Deprecation, and Migration* section, but elsewhere in the
> > KIP it's implied that we'll be replacing
> > existing config which would break any applications that currently rely on
> > it. Please update the *Public Interfaces*
> > and *Proposed Changes* sections to clarify that both of these configs
> will
> > be deprecated.
> >
> > 3) At the end of this section you raise a question that I don't think I
> > quite understand, can you elaborate on this:
> >
> > We can maybe enforce the removal of the deprecated configs and then
> enforce
> > > users?
> > >
> > Note: you only need to worry about deprecating these configs in the
> current
> > KIP. Once deprecated we just need to
> > give users enough time to migrate over to the new API, and then we can
> > remove them in the next major version.
> > The important thing for the KIP itself is just to make sure the change is
> > visible to users and provides a clear
> > migration path.
> >
> > 4) For the Console Consumer you say
> >
> > It would be mandatory to pass windowed.deserializer.inner.class and
> > > window.size.ms config. <Need to check how to do this>
> > >
> >
> > As I said above, you don't need to worry about putting how you'll
> implement
> > this in the KIP document.  But to answer
> > your implicit question, I actually don't think you need to do anything
> > special for the  console consumer -- the
> > TimeWindowedDeserializer itself will verify that both its parameters have
> > been set and throw an exception if not.
> >
> > 5) One last small suggestion: I think we should throw an exception if a
> > user has tried to use the new
> > *windowed.deserializer.inner.class *config in their Kafka Streams
> > application, since it's intended for use only
> > with/for the plain consumer client. If you agree, this is something that
> > should be noted in the KIP somewhere.
> > It may also be a good idea to note this in the config doc string as well,
> > so users don't try to use it as if it was a
> > literal replacement of the *deprecated
> > default.windowed.key.serde.inner* config.
> > What do you think?
> > (To clarify, I'm suggesting we check inside the KafkaStreams constructor
> > and throw if this config is set, but this
> > last bit doesn't need to go into the KIP as it's an implementation
> detail)
> >
> >
> > Once you've addressed the above and cleaned the KIP up a bit, I'm +1 --
> the
> > changes you propose make sense to me.
> >
> > Cheers,
> > Sophie
> >
> > On Mon, Mar 22, 2021 at 3:28 AM Sagar <sa...@gmail.com> wrote:
> >
> > > Hi All,
> > >
> > > I would like to start a discussion on the following KIP:
> > >
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=177047930
> > >
> > > Thanks!
> > > Sagar.
> > >
> >
>

Re: [DISCUSS] KIP-725: Streamlining configurations for TimeWindowedDeserializer.

Posted by Sagar <sa...@gmail.com>.

Thanks Sophie,

Most of these changes should be there in my PR. I will wait for a couple of
days into next week to see if concerns are raised.

Thanks!
Sagar.

On Sat, Apr 24, 2021 at 1:07 AM Sophie Blee-Goldman
<so...@confluent.io.invalid> wrote:

> Thanks Sagar, these changes make sense to me. I don't think we need to call
> for a
> new vote unless there are any concerns raised, but I feel this is probably
> not
> too controversial.
>
> Thanks for the update
>
> On Fri, Apr 23, 2021 at 3:17 AM Sagar <sa...@gmail.com> wrote:
>
> > Hi All,
> >
> > After working on the changes proposed in the KIP, it was pointed out by
> > Sophie that the KIP needs to be upgraded to include Serialisers as well.
> In
> > line with this, I. have updated the KIP with the following changes:
> >
> > 1) Renamed the newly proposed config window.inner.class.deserialiser to
> > windowed.inner.class.serde. 2 changes here are => changed window to
> > windowed and replaced deserialiser with serde. The second change will
> > ensure the config works for both serialisers and deserialisers.
> > 2).Added  better formatting in the page to make it more readable.
> >
> > i am not sure if we will need a new vote for these so please let me know!
> >
> > Thanks!
> > Sagar.
> >
> > On Sun, Apr 4, 2021 at 7:55 AM Sophie Blee-Goldman
> > <so...@confluent.io.invalid> wrote:
> >
> > > I think you can start the vote, we can always return to the discussion
> if
> > > someone raises a
> > > concern during voting.
> > >
> > > Anyways I think/hope this won't be too controversial since we went
> > through
> > > and agreed on
> > > a similar interface not long ago with KIP-659
> > > <
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-659%3A+Improve+TimeWindowedDeserializer+and+TimeWindowedSerde+to+handle+window+size
> > > >
> > >
> > > On Sat, Apr 3, 2021 at 4:26 AM Sagar <sa...@gmail.com>
> wrote:
> > >
> > > > Thanks Leah/Sophie.
> > > >
> > > > I have updated the KIP with the new config.
> > > >
> > > > Do you think we can proceed with the voting process for the KIP or
> > should
> > > > we wait a bit longer?
> > > >
> > > > Thanks!
> > > > Sagar.
> > > >
> > > > On Fri, Apr 2, 2021 at 11:59 PM Sophie Blee-Goldman
> > > > <so...@confluent.io.invalid> wrote:
> > > >
> > > > > Yeah sure, window.inner.class.deserializer sounds good to me
> > > > >
> > > > > On Fri, Apr 2, 2021 at 11:28 AM Leah Thomas
> > > <lthomas@confluent.io.invalid
> > > > >
> > > > > wrote:
> > > > >
> > > > > > Hey Sagar,
> > > > > >
> > > > > > Thanks for picking this up! The proposal looks good to me after
> > > Sophie
> > > > > and
> > > > > > John's changes.
> > > > > >
> > > > > > Cheers,
> > > > > > Leah
> > > > > >
> > > > > > On Fri, Apr 2, 2021 at 6:21 AM Sagar <sa...@gmail.com>
> > > > wrote:
> > > > > >
> > > > > > > Thanks John!
> > > > > > >
> > > > > > > Yeah I think window.inner.class.deserializer sounds good. Your
> > > > thoughts
> > > > > > > @Sophie?
> > > > > > >
> > > > > > > Thanks!
> > > > > > > Sagar.
> > > > > > >
> > > > > > >
> > > > > > > On Fri, Apr 2, 2021 at 5:23 AM John Roesler <
> vvcephei@apache.org
> > >
> > > > > wrote:
> > > > > > >
> > > > > > > > Hi Sagar,
> > > > > > > >
> > > > > > > > I think this is a good proposal.
> > > > > > > >
> > > > > > > > The only change I might recommend is to change the config to
> > more
> > > > > > closely
> > > > > > > > align with the other one, for example:
> > > > > > “window.inner.class.deserializer”
> > > > > > > >
> > > > > > > > But it’s very minor; I leave it to your judgement.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > John
> > > > > > > >
> > > > > > > > On Fri, Mar 26, 2021, at 03:36, Sagar wrote:
> > > > > > > > > Hi Sophie,
> > > > > > > > >
> > > > > > > > > Thanks for the feedback! I have updated the KIP inline with
> > > > > whatever
> > > > > > > you
> > > > > > > > > suggested.
> > > > > > > > >
> > > > > > > > > Regarding point 5, I have added the note as it makes sense
> to
> > > not
> > > > > set
> > > > > > > the
> > > > > > > > > config via the KafkaStreams app.
> > > > > > > > >
> > > > > > > > > Thanks!
> > > > > > > > > Sagar.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Wed, Mar 24, 2021 at 7:52 AM Sophie Blee-Goldman
> > > > > > > > > <so...@confluent.io.invalid> wrote:
> > > > > > > > >
> > > > > > > > > > Hey Sagar,
> > > > > > > > > >
> > > > > > > > > > Thanks for the KIP! The overall proposal looks good to
> me,
> > > but
> > > > I
> > > > > > had
> > > > > > > > some
> > > > > > > > > > miscellaneous notes:
> > > > > > > > > >
> > > > > > > > > > 1) Some general KIP-writing advice:
> > > > > > > > > >     - You don't need to list any implementation details,
> > only
> > > > > > public
> > > > > > > > > > interfaces that are being added, modified, or
> > > > > > > > > >       deprecated. It's better to describe your changes in
> > > > words,
> > > > > or
> > > > > > > > > > occasionally psuedo-code for more complicated
> > > > > > > > > >       changes or algorithms. The KIP is a public
> contract,
> > so
> > > > you
> > > > > > > > generally
> > > > > > > > > > want to agree upon the high-level
> > > > > > > > > >       proposal and avoid getting locked in to specific
> code
> > > > which
> > > > > > you
> > > > > > > > may
> > > > > > > > > > end up wanting to change once you
> > > > > > > > > >       start on the PR.
> > > > > > > > > >       In this KIP, you can remove the code block under
> > > > > > > > > > *TimeWindowedDeserializer
> > > > > > > > > > *and just describe the desired
> > > > > > > > > >       semantics: eg that you should only use the
> > constructor
> > > > > within
> > > > > > > > > > Streams, the configs within the console consumer,
> > > > > > > > > >       or either for a plain consumer client provided they
> > > > match.
> > > > > > > > > >       The code block under *StreamsConfig *however is a
> > good
> > > > > > example
> > > > > > > of
> > > > > > > > > > what should be in a KIP. Only one nit:
> > > > > > > > > >       in the doc string, avoid saying "windowed key" and
> > just
> > > > say
> > > > > > > > "windowed
> > > > > > > > > > record" or something like that.
> > > > > > > > > >     - The *Motivation* section should be focused on any
> > > > > background
> > > > > > or
> > > > > > > > > > additional context that's necessary to
> > > > > > > > > >       understand the KIP, as well as the actual
> motivation
> > > for
> > > > > the
> > > > > > > > change
> > > > > > > > > > being proposed. So here, everything under
> > > > > > > > > >       the second bullet which begins with "The KIP also
> > > > > introduces
> > > > > > > > > > changes..." should be cut from that section and
> > > > > > > > > >       discussed elsewhere.
> > > > > > > > > >     - The *Proposed Changes* and *Public Interfaces*
> > sections
> > > > > have
> > > > > > a
> > > > > > > > lot of
> > > > > > > > > > overlap and repeated content. To be
> > > > > > > > > >       honest I personally have struggled with deciding
> > which
> > > > > > section
> > > > > > > to
> > > > > > > > > > include something in, but a good rule of thumb
> > > > > > > > > >       is to describe the actual changes in the *Proposed
> > > > Changes*
> > > > > > > > section,
> > > > > > > > > > and then use the *Public Interfaces* section
> > > > > > > > > >       to list any new or modified public APIs. In this
> > case,
> > > I
> > > > > > would
> > > > > > > > move
> > > > > > > > > > everything to the *Proposed Changes* section
> > > > > > > > > >       except for the code block with the deprecated
> > config(s)
> > > > and
> > > > > > the
> > > > > > > > new
> > > > > > > > > > config + doc string.
> > > > > > > > > >
> > > > > > > > > > 2) Can you make it clear that both
> > > > > > *default.windowed.key.serde.inner*
> > > > > > > > and
> > > > > > > > > > *default.windowed.key.serde.inner *
> > > > > > > > > > are being deprecated, and we're adding a new config
> called
> > > > > > > > > > *windowed.deserializer.inner.class*, not literally
> > > > > > > > > > renaming the existing *default.windowed.key.serde.inner*
> > > > config?
> > > > > I
> > > > > > > > think
> > > > > > > > > > you're hinting at this in the
> > > > > > > > > > *Compatibility, Deprecation, and Migration* section, but
> > > > > elsewhere
> > > > > > in
> > > > > > > > the
> > > > > > > > > > KIP it's implied that we'll be replacing
> > > > > > > > > > existing config which would break any applications that
> > > > currently
> > > > > > > rely
> > > > > > > > on
> > > > > > > > > > it. Please update the *Public Interfaces*
> > > > > > > > > > and *Proposed Changes* sections to clarify that both of
> > these
> > > > > > configs
> > > > > > > > will
> > > > > > > > > > be deprecated.
> > > > > > > > > >
> > > > > > > > > > 3) At the end of this section you raise a question that I
> > > don't
> > > > > > > think I
> > > > > > > > > > quite understand, can you elaborate on this:
> > > > > > > > > >
> > > > > > > > > > We can maybe enforce the removal of the deprecated
> configs
> > > and
> > > > > then
> > > > > > > > enforce
> > > > > > > > > > > users?
> > > > > > > > > > >
> > > > > > > > > > Note: you only need to worry about deprecating these
> > configs
> > > in
> > > > > the
> > > > > > > > current
> > > > > > > > > > KIP. Once deprecated we just need to
> > > > > > > > > > give users enough time to migrate over to the new API,
> and
> > > then
> > > > > we
> > > > > > > can
> > > > > > > > > > remove them in the next major version.
> > > > > > > > > > The important thing for the KIP itself is just to make
> sure
> > > the
> > > > > > > change
> > > > > > > > is
> > > > > > > > > > visible to users and provides a clear
> > > > > > > > > > migration path.
> > > > > > > > > >
> > > > > > > > > > 4) For the Console Consumer you say
> > > > > > > > > >
> > > > > > > > > > It would be mandatory to pass
> > > windowed.deserializer.inner.class
> > > > > and
> > > > > > > > > > > window.size.ms config. <Need to check how to do this>
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > As I said above, you don't need to worry about putting
> how
> > > > you'll
> > > > > > > > implement
> > > > > > > > > > this in the KIP document.  But to answer
> > > > > > > > > > your implicit question, I actually don't think you need
> to
> > do
> > > > > > > anything
> > > > > > > > > > special for the  console consumer -- the
> > > > > > > > > > TimeWindowedDeserializer itself will verify that both its
> > > > > > parameters
> > > > > > > > have
> > > > > > > > > > been set and throw an exception if not.
> > > > > > > > > >
> > > > > > > > > > 5) One last small suggestion: I think we should throw an
> > > > > exception
> > > > > > > if a
> > > > > > > > > > user has tried to use the new
> > > > > > > > > > *windowed.deserializer.inner.class *config in their Kafka
> > > > Streams
> > > > > > > > > > application, since it's intended for use only
> > > > > > > > > > with/for the plain consumer client. If you agree, this is
> > > > > something
> > > > > > > > that
> > > > > > > > > > should be noted in the KIP somewhere.
> > > > > > > > > > It may also be a good idea to note this in the config doc
> > > > string
> > > > > as
> > > > > > > > well,
> > > > > > > > > > so users don't try to use it as if it was a
> > > > > > > > > > literal replacement of the *deprecated
> > > > > > > > > > default.windowed.key.serde.inner* config.
> > > > > > > > > > What do you think?
> > > > > > > > > > (To clarify, I'm suggesting we check inside the
> > KafkaStreams
> > > > > > > > constructor
> > > > > > > > > > and throw if this config is set, but this
> > > > > > > > > > last bit doesn't need to go into the KIP as it's an
> > > > > implementation
> > > > > > > > detail)
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Once you've addressed the above and cleaned the KIP up a
> > bit,
> > > > I'm
> > > > > > +1
> > > > > > > > -- the
> > > > > > > > > > changes you propose make sense to me.
> > > > > > > > > >
> > > > > > > > > > Cheers,
> > > > > > > > > > Sophie
> > > > > > > > > >
> > > > > > > > > > On Mon, Mar 22, 2021 at 3:28 AM Sagar <
> > > > sagarmeansocean@gmail.com
> > > > > >
> > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Hi All,
> > > > > > > > > > >
> > > > > > > > > > > I would like to start a discussion on the following
> KIP:
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=177047930
> > > > > > > > > > >
> > > > > > > > > > > Thanks!
> > > > > > > > > > > Sagar.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-725: Streamlining configurations for TimeWindowedDeserializer.

Posted by Sophie Blee-Goldman <so...@confluent.io.INVALID>.

Thanks Sagar, these changes make sense to me. I don't think we need to call
for a
new vote unless there are any concerns raised, but I feel this is probably
not
too controversial.

Thanks for the update

On Fri, Apr 23, 2021 at 3:17 AM Sagar <sa...@gmail.com> wrote:

> Hi All,
>
> After working on the changes proposed in the KIP, it was pointed out by
> Sophie that the KIP needs to be upgraded to include Serialisers as well. In
> line with this, I. have updated the KIP with the following changes:
>
> 1) Renamed the newly proposed config window.inner.class.deserialiser to
> windowed.inner.class.serde. 2 changes here are => changed window to
> windowed and replaced deserialiser with serde. The second change will
> ensure the config works for both serialisers and deserialisers.
> 2).Added  better formatting in the page to make it more readable.
>
> i am not sure if we will need a new vote for these so please let me know!
>
> Thanks!
> Sagar.
>
> On Sun, Apr 4, 2021 at 7:55 AM Sophie Blee-Goldman
> <so...@confluent.io.invalid> wrote:
>
> > I think you can start the vote, we can always return to the discussion if
> > someone raises a
> > concern during voting.
> >
> > Anyways I think/hope this won't be too controversial since we went
> through
> > and agreed on
> > a similar interface not long ago with KIP-659
> > <
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-659%3A+Improve+TimeWindowedDeserializer+and+TimeWindowedSerde+to+handle+window+size
> > >
> >
> > On Sat, Apr 3, 2021 at 4:26 AM Sagar <sa...@gmail.com> wrote:
> >
> > > Thanks Leah/Sophie.
> > >
> > > I have updated the KIP with the new config.
> > >
> > > Do you think we can proceed with the voting process for the KIP or
> should
> > > we wait a bit longer?
> > >
> > > Thanks!
> > > Sagar.
> > >
> > > On Fri, Apr 2, 2021 at 11:59 PM Sophie Blee-Goldman
> > > <so...@confluent.io.invalid> wrote:
> > >
> > > > Yeah sure, window.inner.class.deserializer sounds good to me
> > > >
> > > > On Fri, Apr 2, 2021 at 11:28 AM Leah Thomas
> > <lthomas@confluent.io.invalid
> > > >
> > > > wrote:
> > > >
> > > > > Hey Sagar,
> > > > >
> > > > > Thanks for picking this up! The proposal looks good to me after
> > Sophie
> > > > and
> > > > > John's changes.
> > > > >
> > > > > Cheers,
> > > > > Leah
> > > > >
> > > > > On Fri, Apr 2, 2021 at 6:21 AM Sagar <sa...@gmail.com>
> > > wrote:
> > > > >
> > > > > > Thanks John!
> > > > > >
> > > > > > Yeah I think window.inner.class.deserializer sounds good. Your
> > > thoughts
> > > > > > @Sophie?
> > > > > >
> > > > > > Thanks!
> > > > > > Sagar.
> > > > > >
> > > > > >
> > > > > > On Fri, Apr 2, 2021 at 5:23 AM John Roesler <vvcephei@apache.org
> >
> > > > wrote:
> > > > > >
> > > > > > > Hi Sagar,
> > > > > > >
> > > > > > > I think this is a good proposal.
> > > > > > >
> > > > > > > The only change I might recommend is to change the config to
> more
> > > > > closely
> > > > > > > align with the other one, for example:
> > > > > “window.inner.class.deserializer”
> > > > > > >
> > > > > > > But it’s very minor; I leave it to your judgement.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > John
> > > > > > >
> > > > > > > On Fri, Mar 26, 2021, at 03:36, Sagar wrote:
> > > > > > > > Hi Sophie,
> > > > > > > >
> > > > > > > > Thanks for the feedback! I have updated the KIP inline with
> > > > whatever
> > > > > > you
> > > > > > > > suggested.
> > > > > > > >
> > > > > > > > Regarding point 5, I have added the note as it makes sense to
> > not
> > > > set
> > > > > > the
> > > > > > > > config via the KafkaStreams app.
> > > > > > > >
> > > > > > > > Thanks!
> > > > > > > > Sagar.
> > > > > > > >
> > > > > > > >
> > > > > > > > On Wed, Mar 24, 2021 at 7:52 AM Sophie Blee-Goldman
> > > > > > > > <so...@confluent.io.invalid> wrote:
> > > > > > > >
> > > > > > > > > Hey Sagar,
> > > > > > > > >
> > > > > > > > > Thanks for the KIP! The overall proposal looks good to me,
> > but
> > > I
> > > > > had
> > > > > > > some
> > > > > > > > > miscellaneous notes:
> > > > > > > > >
> > > > > > > > > 1) Some general KIP-writing advice:
> > > > > > > > >     - You don't need to list any implementation details,
> only
> > > > > public
> > > > > > > > > interfaces that are being added, modified, or
> > > > > > > > >       deprecated. It's better to describe your changes in
> > > words,
> > > > or
> > > > > > > > > occasionally psuedo-code for more complicated
> > > > > > > > >       changes or algorithms. The KIP is a public contract,
> so
> > > you
> > > > > > > generally
> > > > > > > > > want to agree upon the high-level
> > > > > > > > >       proposal and avoid getting locked in to specific code
> > > which
> > > > > you
> > > > > > > may
> > > > > > > > > end up wanting to change once you
> > > > > > > > >       start on the PR.
> > > > > > > > >       In this KIP, you can remove the code block under
> > > > > > > > > *TimeWindowedDeserializer
> > > > > > > > > *and just describe the desired
> > > > > > > > >       semantics: eg that you should only use the
> constructor
> > > > within
> > > > > > > > > Streams, the configs within the console consumer,
> > > > > > > > >       or either for a plain consumer client provided they
> > > match.
> > > > > > > > >       The code block under *StreamsConfig *however is a
> good
> > > > > example
> > > > > > of
> > > > > > > > > what should be in a KIP. Only one nit:
> > > > > > > > >       in the doc string, avoid saying "windowed key" and
> just
> > > say
> > > > > > > "windowed
> > > > > > > > > record" or something like that.
> > > > > > > > >     - The *Motivation* section should be focused on any
> > > > background
> > > > > or
> > > > > > > > > additional context that's necessary to
> > > > > > > > >       understand the KIP, as well as the actual motivation
> > for
> > > > the
> > > > > > > change
> > > > > > > > > being proposed. So here, everything under
> > > > > > > > >       the second bullet which begins with "The KIP also
> > > > introduces
> > > > > > > > > changes..." should be cut from that section and
> > > > > > > > >       discussed elsewhere.
> > > > > > > > >     - The *Proposed Changes* and *Public Interfaces*
> sections
> > > > have
> > > > > a
> > > > > > > lot of
> > > > > > > > > overlap and repeated content. To be
> > > > > > > > >       honest I personally have struggled with deciding
> which
> > > > > section
> > > > > > to
> > > > > > > > > include something in, but a good rule of thumb
> > > > > > > > >       is to describe the actual changes in the *Proposed
> > > Changes*
> > > > > > > section,
> > > > > > > > > and then use the *Public Interfaces* section
> > > > > > > > >       to list any new or modified public APIs. In this
> case,
> > I
> > > > > would
> > > > > > > move
> > > > > > > > > everything to the *Proposed Changes* section
> > > > > > > > >       except for the code block with the deprecated
> config(s)
> > > and
> > > > > the
> > > > > > > new
> > > > > > > > > config + doc string.
> > > > > > > > >
> > > > > > > > > 2) Can you make it clear that both
> > > > > *default.windowed.key.serde.inner*
> > > > > > > and
> > > > > > > > > *default.windowed.key.serde.inner *
> > > > > > > > > are being deprecated, and we're adding a new config called
> > > > > > > > > *windowed.deserializer.inner.class*, not literally
> > > > > > > > > renaming the existing *default.windowed.key.serde.inner*
> > > config?
> > > > I
> > > > > > > think
> > > > > > > > > you're hinting at this in the
> > > > > > > > > *Compatibility, Deprecation, and Migration* section, but
> > > > elsewhere
> > > > > in
> > > > > > > the
> > > > > > > > > KIP it's implied that we'll be replacing
> > > > > > > > > existing config which would break any applications that
> > > currently
> > > > > > rely
> > > > > > > on
> > > > > > > > > it. Please update the *Public Interfaces*
> > > > > > > > > and *Proposed Changes* sections to clarify that both of
> these
> > > > > configs
> > > > > > > will
> > > > > > > > > be deprecated.
> > > > > > > > >
> > > > > > > > > 3) At the end of this section you raise a question that I
> > don't
> > > > > > think I
> > > > > > > > > quite understand, can you elaborate on this:
> > > > > > > > >
> > > > > > > > > We can maybe enforce the removal of the deprecated configs
> > and
> > > > then
> > > > > > > enforce
> > > > > > > > > > users?
> > > > > > > > > >
> > > > > > > > > Note: you only need to worry about deprecating these
> configs
> > in
> > > > the
> > > > > > > current
> > > > > > > > > KIP. Once deprecated we just need to
> > > > > > > > > give users enough time to migrate over to the new API, and
> > then
> > > > we
> > > > > > can
> > > > > > > > > remove them in the next major version.
> > > > > > > > > The important thing for the KIP itself is just to make sure
> > the
> > > > > > change
> > > > > > > is
> > > > > > > > > visible to users and provides a clear
> > > > > > > > > migration path.
> > > > > > > > >
> > > > > > > > > 4) For the Console Consumer you say
> > > > > > > > >
> > > > > > > > > It would be mandatory to pass
> > windowed.deserializer.inner.class
> > > > and
> > > > > > > > > > window.size.ms config. <Need to check how to do this>
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > As I said above, you don't need to worry about putting how
> > > you'll
> > > > > > > implement
> > > > > > > > > this in the KIP document.  But to answer
> > > > > > > > > your implicit question, I actually don't think you need to
> do
> > > > > > anything
> > > > > > > > > special for the  console consumer -- the
> > > > > > > > > TimeWindowedDeserializer itself will verify that both its
> > > > > parameters
> > > > > > > have
> > > > > > > > > been set and throw an exception if not.
> > > > > > > > >
> > > > > > > > > 5) One last small suggestion: I think we should throw an
> > > > exception
> > > > > > if a
> > > > > > > > > user has tried to use the new
> > > > > > > > > *windowed.deserializer.inner.class *config in their Kafka
> > > Streams
> > > > > > > > > application, since it's intended for use only
> > > > > > > > > with/for the plain consumer client. If you agree, this is
> > > > something
> > > > > > > that
> > > > > > > > > should be noted in the KIP somewhere.
> > > > > > > > > It may also be a good idea to note this in the config doc
> > > string
> > > > as
> > > > > > > well,
> > > > > > > > > so users don't try to use it as if it was a
> > > > > > > > > literal replacement of the *deprecated
> > > > > > > > > default.windowed.key.serde.inner* config.
> > > > > > > > > What do you think?
> > > > > > > > > (To clarify, I'm suggesting we check inside the
> KafkaStreams
> > > > > > > constructor
> > > > > > > > > and throw if this config is set, but this
> > > > > > > > > last bit doesn't need to go into the KIP as it's an
> > > > implementation
> > > > > > > detail)
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Once you've addressed the above and cleaned the KIP up a
> bit,
> > > I'm
> > > > > +1
> > > > > > > -- the
> > > > > > > > > changes you propose make sense to me.
> > > > > > > > >
> > > > > > > > > Cheers,
> > > > > > > > > Sophie
> > > > > > > > >
> > > > > > > > > On Mon, Mar 22, 2021 at 3:28 AM Sagar <
> > > sagarmeansocean@gmail.com
> > > > >
> > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi All,
> > > > > > > > > >
> > > > > > > > > > I would like to start a discussion on the following KIP:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=177047930
> > > > > > > > > >
> > > > > > > > > > Thanks!
> > > > > > > > > > Sagar.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-725: Streamlining configurations for TimeWindowedDeserializer.

Posted by Sagar <sa...@gmail.com>.

Hi All,

After working on the changes proposed in the KIP, it was pointed out by
Sophie that the KIP needs to be upgraded to include Serialisers as well. In
line with this, I. have updated the KIP with the following changes:

1) Renamed the newly proposed config window.inner.class.deserialiser to
windowed.inner.class.serde. 2 changes here are => changed window to
windowed and replaced deserialiser with serde. The second change will
ensure the config works for both serialisers and deserialisers.
2).Added  better formatting in the page to make it more readable.

i am not sure if we will need a new vote for these so please let me know!

Thanks!
Sagar.

On Sun, Apr 4, 2021 at 7:55 AM Sophie Blee-Goldman
<so...@confluent.io.invalid> wrote:

> I think you can start the vote, we can always return to the discussion if
> someone raises a
> concern during voting.
>
> Anyways I think/hope this won't be too controversial since we went through
> and agreed on
> a similar interface not long ago with KIP-659
> <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-659%3A+Improve+TimeWindowedDeserializer+and+TimeWindowedSerde+to+handle+window+size
> >
>
> On Sat, Apr 3, 2021 at 4:26 AM Sagar <sa...@gmail.com> wrote:
>
> > Thanks Leah/Sophie.
> >
> > I have updated the KIP with the new config.
> >
> > Do you think we can proceed with the voting process for the KIP or should
> > we wait a bit longer?
> >
> > Thanks!
> > Sagar.
> >
> > On Fri, Apr 2, 2021 at 11:59 PM Sophie Blee-Goldman
> > <so...@confluent.io.invalid> wrote:
> >
> > > Yeah sure, window.inner.class.deserializer sounds good to me
> > >
> > > On Fri, Apr 2, 2021 at 11:28 AM Leah Thomas
> <lthomas@confluent.io.invalid
> > >
> > > wrote:
> > >
> > > > Hey Sagar,
> > > >
> > > > Thanks for picking this up! The proposal looks good to me after
> Sophie
> > > and
> > > > John's changes.
> > > >
> > > > Cheers,
> > > > Leah
> > > >
> > > > On Fri, Apr 2, 2021 at 6:21 AM Sagar <sa...@gmail.com>
> > wrote:
> > > >
> > > > > Thanks John!
> > > > >
> > > > > Yeah I think window.inner.class.deserializer sounds good. Your
> > thoughts
> > > > > @Sophie?
> > > > >
> > > > > Thanks!
> > > > > Sagar.
> > > > >
> > > > >
> > > > > On Fri, Apr 2, 2021 at 5:23 AM John Roesler <vv...@apache.org>
> > > wrote:
> > > > >
> > > > > > Hi Sagar,
> > > > > >
> > > > > > I think this is a good proposal.
> > > > > >
> > > > > > The only change I might recommend is to change the config to more
> > > > closely
> > > > > > align with the other one, for example:
> > > > “window.inner.class.deserializer”
> > > > > >
> > > > > > But it’s very minor; I leave it to your judgement.
> > > > > >
> > > > > > Thanks,
> > > > > > John
> > > > > >
> > > > > > On Fri, Mar 26, 2021, at 03:36, Sagar wrote:
> > > > > > > Hi Sophie,
> > > > > > >
> > > > > > > Thanks for the feedback! I have updated the KIP inline with
> > > whatever
> > > > > you
> > > > > > > suggested.
> > > > > > >
> > > > > > > Regarding point 5, I have added the note as it makes sense to
> not
> > > set
> > > > > the
> > > > > > > config via the KafkaStreams app.
> > > > > > >
> > > > > > > Thanks!
> > > > > > > Sagar.
> > > > > > >
> > > > > > >
> > > > > > > On Wed, Mar 24, 2021 at 7:52 AM Sophie Blee-Goldman
> > > > > > > <so...@confluent.io.invalid> wrote:
> > > > > > >
> > > > > > > > Hey Sagar,
> > > > > > > >
> > > > > > > > Thanks for the KIP! The overall proposal looks good to me,
> but
> > I
> > > > had
> > > > > > some
> > > > > > > > miscellaneous notes:
> > > > > > > >
> > > > > > > > 1) Some general KIP-writing advice:
> > > > > > > >     - You don't need to list any implementation details, only
> > > > public
> > > > > > > > interfaces that are being added, modified, or
> > > > > > > >       deprecated. It's better to describe your changes in
> > words,
> > > or
> > > > > > > > occasionally psuedo-code for more complicated
> > > > > > > >       changes or algorithms. The KIP is a public contract, so
> > you
> > > > > > generally
> > > > > > > > want to agree upon the high-level
> > > > > > > >       proposal and avoid getting locked in to specific code
> > which
> > > > you
> > > > > > may
> > > > > > > > end up wanting to change once you
> > > > > > > >       start on the PR.
> > > > > > > >       In this KIP, you can remove the code block under
> > > > > > > > *TimeWindowedDeserializer
> > > > > > > > *and just describe the desired
> > > > > > > >       semantics: eg that you should only use the constructor
> > > within
> > > > > > > > Streams, the configs within the console consumer,
> > > > > > > >       or either for a plain consumer client provided they
> > match.
> > > > > > > >       The code block under *StreamsConfig *however is a good
> > > > example
> > > > > of
> > > > > > > > what should be in a KIP. Only one nit:
> > > > > > > >       in the doc string, avoid saying "windowed key" and just
> > say
> > > > > > "windowed
> > > > > > > > record" or something like that.
> > > > > > > >     - The *Motivation* section should be focused on any
> > > background
> > > > or
> > > > > > > > additional context that's necessary to
> > > > > > > >       understand the KIP, as well as the actual motivation
> for
> > > the
> > > > > > change
> > > > > > > > being proposed. So here, everything under
> > > > > > > >       the second bullet which begins with "The KIP also
> > > introduces
> > > > > > > > changes..." should be cut from that section and
> > > > > > > >       discussed elsewhere.
> > > > > > > >     - The *Proposed Changes* and *Public Interfaces* sections
> > > have
> > > > a
> > > > > > lot of
> > > > > > > > overlap and repeated content. To be
> > > > > > > >       honest I personally have struggled with deciding which
> > > > section
> > > > > to
> > > > > > > > include something in, but a good rule of thumb
> > > > > > > >       is to describe the actual changes in the *Proposed
> > Changes*
> > > > > > section,
> > > > > > > > and then use the *Public Interfaces* section
> > > > > > > >       to list any new or modified public APIs. In this case,
> I
> > > > would
> > > > > > move
> > > > > > > > everything to the *Proposed Changes* section
> > > > > > > >       except for the code block with the deprecated config(s)
> > and
> > > > the
> > > > > > new
> > > > > > > > config + doc string.
> > > > > > > >
> > > > > > > > 2) Can you make it clear that both
> > > > *default.windowed.key.serde.inner*
> > > > > > and
> > > > > > > > *default.windowed.key.serde.inner *
> > > > > > > > are being deprecated, and we're adding a new config called
> > > > > > > > *windowed.deserializer.inner.class*, not literally
> > > > > > > > renaming the existing *default.windowed.key.serde.inner*
> > config?
> > > I
> > > > > > think
> > > > > > > > you're hinting at this in the
> > > > > > > > *Compatibility, Deprecation, and Migration* section, but
> > > elsewhere
> > > > in
> > > > > > the
> > > > > > > > KIP it's implied that we'll be replacing
> > > > > > > > existing config which would break any applications that
> > currently
> > > > > rely
> > > > > > on
> > > > > > > > it. Please update the *Public Interfaces*
> > > > > > > > and *Proposed Changes* sections to clarify that both of these
> > > > configs
> > > > > > will
> > > > > > > > be deprecated.
> > > > > > > >
> > > > > > > > 3) At the end of this section you raise a question that I
> don't
> > > > > think I
> > > > > > > > quite understand, can you elaborate on this:
> > > > > > > >
> > > > > > > > We can maybe enforce the removal of the deprecated configs
> and
> > > then
> > > > > > enforce
> > > > > > > > > users?
> > > > > > > > >
> > > > > > > > Note: you only need to worry about deprecating these configs
> in
> > > the
> > > > > > current
> > > > > > > > KIP. Once deprecated we just need to
> > > > > > > > give users enough time to migrate over to the new API, and
> then
> > > we
> > > > > can
> > > > > > > > remove them in the next major version.
> > > > > > > > The important thing for the KIP itself is just to make sure
> the
> > > > > change
> > > > > > is
> > > > > > > > visible to users and provides a clear
> > > > > > > > migration path.
> > > > > > > >
> > > > > > > > 4) For the Console Consumer you say
> > > > > > > >
> > > > > > > > It would be mandatory to pass
> windowed.deserializer.inner.class
> > > and
> > > > > > > > > window.size.ms config. <Need to check how to do this>
> > > > > > > > >
> > > > > > > >
> > > > > > > > As I said above, you don't need to worry about putting how
> > you'll
> > > > > > implement
> > > > > > > > this in the KIP document.  But to answer
> > > > > > > > your implicit question, I actually don't think you need to do
> > > > > anything
> > > > > > > > special for the  console consumer -- the
> > > > > > > > TimeWindowedDeserializer itself will verify that both its
> > > > parameters
> > > > > > have
> > > > > > > > been set and throw an exception if not.
> > > > > > > >
> > > > > > > > 5) One last small suggestion: I think we should throw an
> > > exception
> > > > > if a
> > > > > > > > user has tried to use the new
> > > > > > > > *windowed.deserializer.inner.class *config in their Kafka
> > Streams
> > > > > > > > application, since it's intended for use only
> > > > > > > > with/for the plain consumer client. If you agree, this is
> > > something
> > > > > > that
> > > > > > > > should be noted in the KIP somewhere.
> > > > > > > > It may also be a good idea to note this in the config doc
> > string
> > > as
> > > > > > well,
> > > > > > > > so users don't try to use it as if it was a
> > > > > > > > literal replacement of the *deprecated
> > > > > > > > default.windowed.key.serde.inner* config.
> > > > > > > > What do you think?
> > > > > > > > (To clarify, I'm suggesting we check inside the KafkaStreams
> > > > > > constructor
> > > > > > > > and throw if this config is set, but this
> > > > > > > > last bit doesn't need to go into the KIP as it's an
> > > implementation
> > > > > > detail)
> > > > > > > >
> > > > > > > >
> > > > > > > > Once you've addressed the above and cleaned the KIP up a bit,
> > I'm
> > > > +1
> > > > > > -- the
> > > > > > > > changes you propose make sense to me.
> > > > > > > >
> > > > > > > > Cheers,
> > > > > > > > Sophie
> > > > > > > >
> > > > > > > > On Mon, Mar 22, 2021 at 3:28 AM Sagar <
> > sagarmeansocean@gmail.com
> > > >
> > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi All,
> > > > > > > > >
> > > > > > > > > I would like to start a discussion on the following KIP:
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=177047930
> > > > > > > > >
> > > > > > > > > Thanks!
> > > > > > > > > Sagar.
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-725: Streamlining configurations for TimeWindowedDeserializer.

Posted by Sophie Blee-Goldman <so...@confluent.io.INVALID>.

I think you can start the vote, we can always return to the discussion if
someone raises a
concern during voting.

Anyways I think/hope this won't be too controversial since we went through
and agreed on
a similar interface not long ago with KIP-659
<https://cwiki.apache.org/confluence/display/KAFKA/KIP-659%3A+Improve+TimeWindowedDeserializer+and+TimeWindowedSerde+to+handle+window+size>

On Sat, Apr 3, 2021 at 4:26 AM Sagar <sa...@gmail.com> wrote:

> Thanks Leah/Sophie.
>
> I have updated the KIP with the new config.
>
> Do you think we can proceed with the voting process for the KIP or should
> we wait a bit longer?
>
> Thanks!
> Sagar.
>
> On Fri, Apr 2, 2021 at 11:59 PM Sophie Blee-Goldman
> <so...@confluent.io.invalid> wrote:
>
> > Yeah sure, window.inner.class.deserializer sounds good to me
> >
> > On Fri, Apr 2, 2021 at 11:28 AM Leah Thomas <lthomas@confluent.io.invalid
> >
> > wrote:
> >
> > > Hey Sagar,
> > >
> > > Thanks for picking this up! The proposal looks good to me after Sophie
> > and
> > > John's changes.
> > >
> > > Cheers,
> > > Leah
> > >
> > > On Fri, Apr 2, 2021 at 6:21 AM Sagar <sa...@gmail.com>
> wrote:
> > >
> > > > Thanks John!
> > > >
> > > > Yeah I think window.inner.class.deserializer sounds good. Your
> thoughts
> > > > @Sophie?
> > > >
> > > > Thanks!
> > > > Sagar.
> > > >
> > > >
> > > > On Fri, Apr 2, 2021 at 5:23 AM John Roesler <vv...@apache.org>
> > wrote:
> > > >
> > > > > Hi Sagar,
> > > > >
> > > > > I think this is a good proposal.
> > > > >
> > > > > The only change I might recommend is to change the config to more
> > > closely
> > > > > align with the other one, for example:
> > > “window.inner.class.deserializer”
> > > > >
> > > > > But it’s very minor; I leave it to your judgement.
> > > > >
> > > > > Thanks,
> > > > > John
> > > > >
> > > > > On Fri, Mar 26, 2021, at 03:36, Sagar wrote:
> > > > > > Hi Sophie,
> > > > > >
> > > > > > Thanks for the feedback! I have updated the KIP inline with
> > whatever
> > > > you
> > > > > > suggested.
> > > > > >
> > > > > > Regarding point 5, I have added the note as it makes sense to not
> > set
> > > > the
> > > > > > config via the KafkaStreams app.
> > > > > >
> > > > > > Thanks!
> > > > > > Sagar.
> > > > > >
> > > > > >
> > > > > > On Wed, Mar 24, 2021 at 7:52 AM Sophie Blee-Goldman
> > > > > > <so...@confluent.io.invalid> wrote:
> > > > > >
> > > > > > > Hey Sagar,
> > > > > > >
> > > > > > > Thanks for the KIP! The overall proposal looks good to me, but
> I
> > > had
> > > > > some
> > > > > > > miscellaneous notes:
> > > > > > >
> > > > > > > 1) Some general KIP-writing advice:
> > > > > > >     - You don't need to list any implementation details, only
> > > public
> > > > > > > interfaces that are being added, modified, or
> > > > > > >       deprecated. It's better to describe your changes in
> words,
> > or
> > > > > > > occasionally psuedo-code for more complicated
> > > > > > >       changes or algorithms. The KIP is a public contract, so
> you
> > > > > generally
> > > > > > > want to agree upon the high-level
> > > > > > >       proposal and avoid getting locked in to specific code
> which
> > > you
> > > > > may
> > > > > > > end up wanting to change once you
> > > > > > >       start on the PR.
> > > > > > >       In this KIP, you can remove the code block under
> > > > > > > *TimeWindowedDeserializer
> > > > > > > *and just describe the desired
> > > > > > >       semantics: eg that you should only use the constructor
> > within
> > > > > > > Streams, the configs within the console consumer,
> > > > > > >       or either for a plain consumer client provided they
> match.
> > > > > > >       The code block under *StreamsConfig *however is a good
> > > example
> > > > of
> > > > > > > what should be in a KIP. Only one nit:
> > > > > > >       in the doc string, avoid saying "windowed key" and just
> say
> > > > > "windowed
> > > > > > > record" or something like that.
> > > > > > >     - The *Motivation* section should be focused on any
> > background
> > > or
> > > > > > > additional context that's necessary to
> > > > > > >       understand the KIP, as well as the actual motivation for
> > the
> > > > > change
> > > > > > > being proposed. So here, everything under
> > > > > > >       the second bullet which begins with "The KIP also
> > introduces
> > > > > > > changes..." should be cut from that section and
> > > > > > >       discussed elsewhere.
> > > > > > >     - The *Proposed Changes* and *Public Interfaces* sections
> > have
> > > a
> > > > > lot of
> > > > > > > overlap and repeated content. To be
> > > > > > >       honest I personally have struggled with deciding which
> > > section
> > > > to
> > > > > > > include something in, but a good rule of thumb
> > > > > > >       is to describe the actual changes in the *Proposed
> Changes*
> > > > > section,
> > > > > > > and then use the *Public Interfaces* section
> > > > > > >       to list any new or modified public APIs. In this case, I
> > > would
> > > > > move
> > > > > > > everything to the *Proposed Changes* section
> > > > > > >       except for the code block with the deprecated config(s)
> and
> > > the
> > > > > new
> > > > > > > config + doc string.
> > > > > > >
> > > > > > > 2) Can you make it clear that both
> > > *default.windowed.key.serde.inner*
> > > > > and
> > > > > > > *default.windowed.key.serde.inner *
> > > > > > > are being deprecated, and we're adding a new config called
> > > > > > > *windowed.deserializer.inner.class*, not literally
> > > > > > > renaming the existing *default.windowed.key.serde.inner*
> config?
> > I
> > > > > think
> > > > > > > you're hinting at this in the
> > > > > > > *Compatibility, Deprecation, and Migration* section, but
> > elsewhere
> > > in
> > > > > the
> > > > > > > KIP it's implied that we'll be replacing
> > > > > > > existing config which would break any applications that
> currently
> > > > rely
> > > > > on
> > > > > > > it. Please update the *Public Interfaces*
> > > > > > > and *Proposed Changes* sections to clarify that both of these
> > > configs
> > > > > will
> > > > > > > be deprecated.
> > > > > > >
> > > > > > > 3) At the end of this section you raise a question that I don't
> > > > think I
> > > > > > > quite understand, can you elaborate on this:
> > > > > > >
> > > > > > > We can maybe enforce the removal of the deprecated configs and
> > then
> > > > > enforce
> > > > > > > > users?
> > > > > > > >
> > > > > > > Note: you only need to worry about deprecating these configs in
> > the
> > > > > current
> > > > > > > KIP. Once deprecated we just need to
> > > > > > > give users enough time to migrate over to the new API, and then
> > we
> > > > can
> > > > > > > remove them in the next major version.
> > > > > > > The important thing for the KIP itself is just to make sure the
> > > > change
> > > > > is
> > > > > > > visible to users and provides a clear
> > > > > > > migration path.
> > > > > > >
> > > > > > > 4) For the Console Consumer you say
> > > > > > >
> > > > > > > It would be mandatory to pass windowed.deserializer.inner.class
> > and
> > > > > > > > window.size.ms config. <Need to check how to do this>
> > > > > > > >
> > > > > > >
> > > > > > > As I said above, you don't need to worry about putting how
> you'll
> > > > > implement
> > > > > > > this in the KIP document.  But to answer
> > > > > > > your implicit question, I actually don't think you need to do
> > > > anything
> > > > > > > special for the  console consumer -- the
> > > > > > > TimeWindowedDeserializer itself will verify that both its
> > > parameters
> > > > > have
> > > > > > > been set and throw an exception if not.
> > > > > > >
> > > > > > > 5) One last small suggestion: I think we should throw an
> > exception
> > > > if a
> > > > > > > user has tried to use the new
> > > > > > > *windowed.deserializer.inner.class *config in their Kafka
> Streams
> > > > > > > application, since it's intended for use only
> > > > > > > with/for the plain consumer client. If you agree, this is
> > something
> > > > > that
> > > > > > > should be noted in the KIP somewhere.
> > > > > > > It may also be a good idea to note this in the config doc
> string
> > as
> > > > > well,
> > > > > > > so users don't try to use it as if it was a
> > > > > > > literal replacement of the *deprecated
> > > > > > > default.windowed.key.serde.inner* config.
> > > > > > > What do you think?
> > > > > > > (To clarify, I'm suggesting we check inside the KafkaStreams
> > > > > constructor
> > > > > > > and throw if this config is set, but this
> > > > > > > last bit doesn't need to go into the KIP as it's an
> > implementation
> > > > > detail)
> > > > > > >
> > > > > > >
> > > > > > > Once you've addressed the above and cleaned the KIP up a bit,
> I'm
> > > +1
> > > > > -- the
> > > > > > > changes you propose make sense to me.
> > > > > > >
> > > > > > > Cheers,
> > > > > > > Sophie
> > > > > > >
> > > > > > > On Mon, Mar 22, 2021 at 3:28 AM Sagar <
> sagarmeansocean@gmail.com
> > >
> > > > > wrote:
> > > > > > >
> > > > > > > > Hi All,
> > > > > > > >
> > > > > > > > I would like to start a discussion on the following KIP:
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=177047930
> > > > > > > >
> > > > > > > > Thanks!
> > > > > > > > Sagar.
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-725: Streamlining configurations for TimeWindowedDeserializer.

Posted by Sagar <sa...@gmail.com>.

Thanks Leah/Sophie.

I have updated the KIP with the new config.

Do you think we can proceed with the voting process for the KIP or should
we wait a bit longer?

Thanks!
Sagar.

On Fri, Apr 2, 2021 at 11:59 PM Sophie Blee-Goldman
<so...@confluent.io.invalid> wrote:

> Yeah sure, window.inner.class.deserializer sounds good to me
>
> On Fri, Apr 2, 2021 at 11:28 AM Leah Thomas <lt...@confluent.io.invalid>
> wrote:
>
> > Hey Sagar,
> >
> > Thanks for picking this up! The proposal looks good to me after Sophie
> and
> > John's changes.
> >
> > Cheers,
> > Leah
> >
> > On Fri, Apr 2, 2021 at 6:21 AM Sagar <sa...@gmail.com> wrote:
> >
> > > Thanks John!
> > >
> > > Yeah I think window.inner.class.deserializer sounds good. Your thoughts
> > > @Sophie?
> > >
> > > Thanks!
> > > Sagar.
> > >
> > >
> > > On Fri, Apr 2, 2021 at 5:23 AM John Roesler <vv...@apache.org>
> wrote:
> > >
> > > > Hi Sagar,
> > > >
> > > > I think this is a good proposal.
> > > >
> > > > The only change I might recommend is to change the config to more
> > closely
> > > > align with the other one, for example:
> > “window.inner.class.deserializer”
> > > >
> > > > But it’s very minor; I leave it to your judgement.
> > > >
> > > > Thanks,
> > > > John
> > > >
> > > > On Fri, Mar 26, 2021, at 03:36, Sagar wrote:
> > > > > Hi Sophie,
> > > > >
> > > > > Thanks for the feedback! I have updated the KIP inline with
> whatever
> > > you
> > > > > suggested.
> > > > >
> > > > > Regarding point 5, I have added the note as it makes sense to not
> set
> > > the
> > > > > config via the KafkaStreams app.
> > > > >
> > > > > Thanks!
> > > > > Sagar.
> > > > >
> > > > >
> > > > > On Wed, Mar 24, 2021 at 7:52 AM Sophie Blee-Goldman
> > > > > <so...@confluent.io.invalid> wrote:
> > > > >
> > > > > > Hey Sagar,
> > > > > >
> > > > > > Thanks for the KIP! The overall proposal looks good to me, but I
> > had
> > > > some
> > > > > > miscellaneous notes:
> > > > > >
> > > > > > 1) Some general KIP-writing advice:
> > > > > >     - You don't need to list any implementation details, only
> > public
> > > > > > interfaces that are being added, modified, or
> > > > > >       deprecated. It's better to describe your changes in words,
> or
> > > > > > occasionally psuedo-code for more complicated
> > > > > >       changes or algorithms. The KIP is a public contract, so you
> > > > generally
> > > > > > want to agree upon the high-level
> > > > > >       proposal and avoid getting locked in to specific code which
> > you
> > > > may
> > > > > > end up wanting to change once you
> > > > > >       start on the PR.
> > > > > >       In this KIP, you can remove the code block under
> > > > > > *TimeWindowedDeserializer
> > > > > > *and just describe the desired
> > > > > >       semantics: eg that you should only use the constructor
> within
> > > > > > Streams, the configs within the console consumer,
> > > > > >       or either for a plain consumer client provided they match.
> > > > > >       The code block under *StreamsConfig *however is a good
> > example
> > > of
> > > > > > what should be in a KIP. Only one nit:
> > > > > >       in the doc string, avoid saying "windowed key" and just say
> > > > "windowed
> > > > > > record" or something like that.
> > > > > >     - The *Motivation* section should be focused on any
> background
> > or
> > > > > > additional context that's necessary to
> > > > > >       understand the KIP, as well as the actual motivation for
> the
> > > > change
> > > > > > being proposed. So here, everything under
> > > > > >       the second bullet which begins with "The KIP also
> introduces
> > > > > > changes..." should be cut from that section and
> > > > > >       discussed elsewhere.
> > > > > >     - The *Proposed Changes* and *Public Interfaces* sections
> have
> > a
> > > > lot of
> > > > > > overlap and repeated content. To be
> > > > > >       honest I personally have struggled with deciding which
> > section
> > > to
> > > > > > include something in, but a good rule of thumb
> > > > > >       is to describe the actual changes in the *Proposed Changes*
> > > > section,
> > > > > > and then use the *Public Interfaces* section
> > > > > >       to list any new or modified public APIs. In this case, I
> > would
> > > > move
> > > > > > everything to the *Proposed Changes* section
> > > > > >       except for the code block with the deprecated config(s) and
> > the
> > > > new
> > > > > > config + doc string.
> > > > > >
> > > > > > 2) Can you make it clear that both
> > *default.windowed.key.serde.inner*
> > > > and
> > > > > > *default.windowed.key.serde.inner *
> > > > > > are being deprecated, and we're adding a new config called
> > > > > > *windowed.deserializer.inner.class*, not literally
> > > > > > renaming the existing *default.windowed.key.serde.inner* config?
> I
> > > > think
> > > > > > you're hinting at this in the
> > > > > > *Compatibility, Deprecation, and Migration* section, but
> elsewhere
> > in
> > > > the
> > > > > > KIP it's implied that we'll be replacing
> > > > > > existing config which would break any applications that currently
> > > rely
> > > > on
> > > > > > it. Please update the *Public Interfaces*
> > > > > > and *Proposed Changes* sections to clarify that both of these
> > configs
> > > > will
> > > > > > be deprecated.
> > > > > >
> > > > > > 3) At the end of this section you raise a question that I don't
> > > think I
> > > > > > quite understand, can you elaborate on this:
> > > > > >
> > > > > > We can maybe enforce the removal of the deprecated configs and
> then
> > > > enforce
> > > > > > > users?
> > > > > > >
> > > > > > Note: you only need to worry about deprecating these configs in
> the
> > > > current
> > > > > > KIP. Once deprecated we just need to
> > > > > > give users enough time to migrate over to the new API, and then
> we
> > > can
> > > > > > remove them in the next major version.
> > > > > > The important thing for the KIP itself is just to make sure the
> > > change
> > > > is
> > > > > > visible to users and provides a clear
> > > > > > migration path.
> > > > > >
> > > > > > 4) For the Console Consumer you say
> > > > > >
> > > > > > It would be mandatory to pass windowed.deserializer.inner.class
> and
> > > > > > > window.size.ms config. <Need to check how to do this>
> > > > > > >
> > > > > >
> > > > > > As I said above, you don't need to worry about putting how you'll
> > > > implement
> > > > > > this in the KIP document.  But to answer
> > > > > > your implicit question, I actually don't think you need to do
> > > anything
> > > > > > special for the  console consumer -- the
> > > > > > TimeWindowedDeserializer itself will verify that both its
> > parameters
> > > > have
> > > > > > been set and throw an exception if not.
> > > > > >
> > > > > > 5) One last small suggestion: I think we should throw an
> exception
> > > if a
> > > > > > user has tried to use the new
> > > > > > *windowed.deserializer.inner.class *config in their Kafka Streams
> > > > > > application, since it's intended for use only
> > > > > > with/for the plain consumer client. If you agree, this is
> something
> > > > that
> > > > > > should be noted in the KIP somewhere.
> > > > > > It may also be a good idea to note this in the config doc string
> as
> > > > well,
> > > > > > so users don't try to use it as if it was a
> > > > > > literal replacement of the *deprecated
> > > > > > default.windowed.key.serde.inner* config.
> > > > > > What do you think?
> > > > > > (To clarify, I'm suggesting we check inside the KafkaStreams
> > > > constructor
> > > > > > and throw if this config is set, but this
> > > > > > last bit doesn't need to go into the KIP as it's an
> implementation
> > > > detail)
> > > > > >
> > > > > >
> > > > > > Once you've addressed the above and cleaned the KIP up a bit, I'm
> > +1
> > > > -- the
> > > > > > changes you propose make sense to me.
> > > > > >
> > > > > > Cheers,
> > > > > > Sophie
> > > > > >
> > > > > > On Mon, Mar 22, 2021 at 3:28 AM Sagar <sagarmeansocean@gmail.com
> >
> > > > wrote:
> > > > > >
> > > > > > > Hi All,
> > > > > > >
> > > > > > > I would like to start a discussion on the following KIP:
> > > > > > >
> > > > > > >
> > > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=177047930
> > > > > > >
> > > > > > > Thanks!
> > > > > > > Sagar.
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-725: Streamlining configurations for TimeWindowedDeserializer.

Posted by Sophie Blee-Goldman <so...@confluent.io.INVALID>.

Yeah sure, window.inner.class.deserializer sounds good to me

On Fri, Apr 2, 2021 at 11:28 AM Leah Thomas <lt...@confluent.io.invalid>
wrote:

> Hey Sagar,
>
> Thanks for picking this up! The proposal looks good to me after Sophie and
> John's changes.
>
> Cheers,
> Leah
>
> On Fri, Apr 2, 2021 at 6:21 AM Sagar <sa...@gmail.com> wrote:
>
> > Thanks John!
> >
> > Yeah I think window.inner.class.deserializer sounds good. Your thoughts
> > @Sophie?
> >
> > Thanks!
> > Sagar.
> >
> >
> > On Fri, Apr 2, 2021 at 5:23 AM John Roesler <vv...@apache.org> wrote:
> >
> > > Hi Sagar,
> > >
> > > I think this is a good proposal.
> > >
> > > The only change I might recommend is to change the config to more
> closely
> > > align with the other one, for example:
> “window.inner.class.deserializer”
> > >
> > > But it’s very minor; I leave it to your judgement.
> > >
> > > Thanks,
> > > John
> > >
> > > On Fri, Mar 26, 2021, at 03:36, Sagar wrote:
> > > > Hi Sophie,
> > > >
> > > > Thanks for the feedback! I have updated the KIP inline with whatever
> > you
> > > > suggested.
> > > >
> > > > Regarding point 5, I have added the note as it makes sense to not set
> > the
> > > > config via the KafkaStreams app.
> > > >
> > > > Thanks!
> > > > Sagar.
> > > >
> > > >
> > > > On Wed, Mar 24, 2021 at 7:52 AM Sophie Blee-Goldman
> > > > <so...@confluent.io.invalid> wrote:
> > > >
> > > > > Hey Sagar,
> > > > >
> > > > > Thanks for the KIP! The overall proposal looks good to me, but I
> had
> > > some
> > > > > miscellaneous notes:
> > > > >
> > > > > 1) Some general KIP-writing advice:
> > > > >     - You don't need to list any implementation details, only
> public
> > > > > interfaces that are being added, modified, or
> > > > >       deprecated. It's better to describe your changes in words, or
> > > > > occasionally psuedo-code for more complicated
> > > > >       changes or algorithms. The KIP is a public contract, so you
> > > generally
> > > > > want to agree upon the high-level
> > > > >       proposal and avoid getting locked in to specific code which
> you
> > > may
> > > > > end up wanting to change once you
> > > > >       start on the PR.
> > > > >       In this KIP, you can remove the code block under
> > > > > *TimeWindowedDeserializer
> > > > > *and just describe the desired
> > > > >       semantics: eg that you should only use the constructor within
> > > > > Streams, the configs within the console consumer,
> > > > >       or either for a plain consumer client provided they match.
> > > > >       The code block under *StreamsConfig *however is a good
> example
> > of
> > > > > what should be in a KIP. Only one nit:
> > > > >       in the doc string, avoid saying "windowed key" and just say
> > > "windowed
> > > > > record" or something like that.
> > > > >     - The *Motivation* section should be focused on any background
> or
> > > > > additional context that's necessary to
> > > > >       understand the KIP, as well as the actual motivation for the
> > > change
> > > > > being proposed. So here, everything under
> > > > >       the second bullet which begins with "The KIP also introduces
> > > > > changes..." should be cut from that section and
> > > > >       discussed elsewhere.
> > > > >     - The *Proposed Changes* and *Public Interfaces* sections have
> a
> > > lot of
> > > > > overlap and repeated content. To be
> > > > >       honest I personally have struggled with deciding which
> section
> > to
> > > > > include something in, but a good rule of thumb
> > > > >       is to describe the actual changes in the *Proposed Changes*
> > > section,
> > > > > and then use the *Public Interfaces* section
> > > > >       to list any new or modified public APIs. In this case, I
> would
> > > move
> > > > > everything to the *Proposed Changes* section
> > > > >       except for the code block with the deprecated config(s) and
> the
> > > new
> > > > > config + doc string.
> > > > >
> > > > > 2) Can you make it clear that both
> *default.windowed.key.serde.inner*
> > > and
> > > > > *default.windowed.key.serde.inner *
> > > > > are being deprecated, and we're adding a new config called
> > > > > *windowed.deserializer.inner.class*, not literally
> > > > > renaming the existing *default.windowed.key.serde.inner* config? I
> > > think
> > > > > you're hinting at this in the
> > > > > *Compatibility, Deprecation, and Migration* section, but elsewhere
> in
> > > the
> > > > > KIP it's implied that we'll be replacing
> > > > > existing config which would break any applications that currently
> > rely
> > > on
> > > > > it. Please update the *Public Interfaces*
> > > > > and *Proposed Changes* sections to clarify that both of these
> configs
> > > will
> > > > > be deprecated.
> > > > >
> > > > > 3) At the end of this section you raise a question that I don't
> > think I
> > > > > quite understand, can you elaborate on this:
> > > > >
> > > > > We can maybe enforce the removal of the deprecated configs and then
> > > enforce
> > > > > > users?
> > > > > >
> > > > > Note: you only need to worry about deprecating these configs in the
> > > current
> > > > > KIP. Once deprecated we just need to
> > > > > give users enough time to migrate over to the new API, and then we
> > can
> > > > > remove them in the next major version.
> > > > > The important thing for the KIP itself is just to make sure the
> > change
> > > is
> > > > > visible to users and provides a clear
> > > > > migration path.
> > > > >
> > > > > 4) For the Console Consumer you say
> > > > >
> > > > > It would be mandatory to pass windowed.deserializer.inner.class and
> > > > > > window.size.ms config. <Need to check how to do this>
> > > > > >
> > > > >
> > > > > As I said above, you don't need to worry about putting how you'll
> > > implement
> > > > > this in the KIP document.  But to answer
> > > > > your implicit question, I actually don't think you need to do
> > anything
> > > > > special for the  console consumer -- the
> > > > > TimeWindowedDeserializer itself will verify that both its
> parameters
> > > have
> > > > > been set and throw an exception if not.
> > > > >
> > > > > 5) One last small suggestion: I think we should throw an exception
> > if a
> > > > > user has tried to use the new
> > > > > *windowed.deserializer.inner.class *config in their Kafka Streams
> > > > > application, since it's intended for use only
> > > > > with/for the plain consumer client. If you agree, this is something
> > > that
> > > > > should be noted in the KIP somewhere.
> > > > > It may also be a good idea to note this in the config doc string as
> > > well,
> > > > > so users don't try to use it as if it was a
> > > > > literal replacement of the *deprecated
> > > > > default.windowed.key.serde.inner* config.
> > > > > What do you think?
> > > > > (To clarify, I'm suggesting we check inside the KafkaStreams
> > > constructor
> > > > > and throw if this config is set, but this
> > > > > last bit doesn't need to go into the KIP as it's an implementation
> > > detail)
> > > > >
> > > > >
> > > > > Once you've addressed the above and cleaned the KIP up a bit, I'm
> +1
> > > -- the
> > > > > changes you propose make sense to me.
> > > > >
> > > > > Cheers,
> > > > > Sophie
> > > > >
> > > > > On Mon, Mar 22, 2021 at 3:28 AM Sagar <sa...@gmail.com>
> > > wrote:
> > > > >
> > > > > > Hi All,
> > > > > >
> > > > > > I would like to start a discussion on the following KIP:
> > > > > >
> > > > > >
> > > > >
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=177047930
> > > > > >
> > > > > > Thanks!
> > > > > > Sagar.
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-725: Streamlining configurations for TimeWindowedDeserializer.

Posted by Leah Thomas <lt...@confluent.io.INVALID>.

Hey Sagar,

Thanks for picking this up! The proposal looks good to me after Sophie and
John's changes.

Cheers,
Leah

On Fri, Apr 2, 2021 at 6:21 AM Sagar <sa...@gmail.com> wrote:

> Thanks John!
>
> Yeah I think window.inner.class.deserializer sounds good. Your thoughts
> @Sophie?
>
> Thanks!
> Sagar.
>
>
> On Fri, Apr 2, 2021 at 5:23 AM John Roesler <vv...@apache.org> wrote:
>
> > Hi Sagar,
> >
> > I think this is a good proposal.
> >
> > The only change I might recommend is to change the config to more closely
> > align with the other one, for example: “window.inner.class.deserializer”
> >
> > But it’s very minor; I leave it to your judgement.
> >
> > Thanks,
> > John
> >
> > On Fri, Mar 26, 2021, at 03:36, Sagar wrote:
> > > Hi Sophie,
> > >
> > > Thanks for the feedback! I have updated the KIP inline with whatever
> you
> > > suggested.
> > >
> > > Regarding point 5, I have added the note as it makes sense to not set
> the
> > > config via the KafkaStreams app.
> > >
> > > Thanks!
> > > Sagar.
> > >
> > >
> > > On Wed, Mar 24, 2021 at 7:52 AM Sophie Blee-Goldman
> > > <so...@confluent.io.invalid> wrote:
> > >
> > > > Hey Sagar,
> > > >
> > > > Thanks for the KIP! The overall proposal looks good to me, but I had
> > some
> > > > miscellaneous notes:
> > > >
> > > > 1) Some general KIP-writing advice:
> > > >     - You don't need to list any implementation details, only public
> > > > interfaces that are being added, modified, or
> > > >       deprecated. It's better to describe your changes in words, or
> > > > occasionally psuedo-code for more complicated
> > > >       changes or algorithms. The KIP is a public contract, so you
> > generally
> > > > want to agree upon the high-level
> > > >       proposal and avoid getting locked in to specific code which you
> > may
> > > > end up wanting to change once you
> > > >       start on the PR.
> > > >       In this KIP, you can remove the code block under
> > > > *TimeWindowedDeserializer
> > > > *and just describe the desired
> > > >       semantics: eg that you should only use the constructor within
> > > > Streams, the configs within the console consumer,
> > > >       or either for a plain consumer client provided they match.
> > > >       The code block under *StreamsConfig *however is a good example
> of
> > > > what should be in a KIP. Only one nit:
> > > >       in the doc string, avoid saying "windowed key" and just say
> > "windowed
> > > > record" or something like that.
> > > >     - The *Motivation* section should be focused on any background or
> > > > additional context that's necessary to
> > > >       understand the KIP, as well as the actual motivation for the
> > change
> > > > being proposed. So here, everything under
> > > >       the second bullet which begins with "The KIP also introduces
> > > > changes..." should be cut from that section and
> > > >       discussed elsewhere.
> > > >     - The *Proposed Changes* and *Public Interfaces* sections have a
> > lot of
> > > > overlap and repeated content. To be
> > > >       honest I personally have struggled with deciding which section
> to
> > > > include something in, but a good rule of thumb
> > > >       is to describe the actual changes in the *Proposed Changes*
> > section,
> > > > and then use the *Public Interfaces* section
> > > >       to list any new or modified public APIs. In this case, I would
> > move
> > > > everything to the *Proposed Changes* section
> > > >       except for the code block with the deprecated config(s) and the
> > new
> > > > config + doc string.
> > > >
> > > > 2) Can you make it clear that both *default.windowed.key.serde.inner*
> > and
> > > > *default.windowed.key.serde.inner *
> > > > are being deprecated, and we're adding a new config called
> > > > *windowed.deserializer.inner.class*, not literally
> > > > renaming the existing *default.windowed.key.serde.inner* config? I
> > think
> > > > you're hinting at this in the
> > > > *Compatibility, Deprecation, and Migration* section, but elsewhere in
> > the
> > > > KIP it's implied that we'll be replacing
> > > > existing config which would break any applications that currently
> rely
> > on
> > > > it. Please update the *Public Interfaces*
> > > > and *Proposed Changes* sections to clarify that both of these configs
> > will
> > > > be deprecated.
> > > >
> > > > 3) At the end of this section you raise a question that I don't
> think I
> > > > quite understand, can you elaborate on this:
> > > >
> > > > We can maybe enforce the removal of the deprecated configs and then
> > enforce
> > > > > users?
> > > > >
> > > > Note: you only need to worry about deprecating these configs in the
> > current
> > > > KIP. Once deprecated we just need to
> > > > give users enough time to migrate over to the new API, and then we
> can
> > > > remove them in the next major version.
> > > > The important thing for the KIP itself is just to make sure the
> change
> > is
> > > > visible to users and provides a clear
> > > > migration path.
> > > >
> > > > 4) For the Console Consumer you say
> > > >
> > > > It would be mandatory to pass windowed.deserializer.inner.class and
> > > > > window.size.ms config. <Need to check how to do this>
> > > > >
> > > >
> > > > As I said above, you don't need to worry about putting how you'll
> > implement
> > > > this in the KIP document.  But to answer
> > > > your implicit question, I actually don't think you need to do
> anything
> > > > special for the  console consumer -- the
> > > > TimeWindowedDeserializer itself will verify that both its parameters
> > have
> > > > been set and throw an exception if not.
> > > >
> > > > 5) One last small suggestion: I think we should throw an exception
> if a
> > > > user has tried to use the new
> > > > *windowed.deserializer.inner.class *config in their Kafka Streams
> > > > application, since it's intended for use only
> > > > with/for the plain consumer client. If you agree, this is something
> > that
> > > > should be noted in the KIP somewhere.
> > > > It may also be a good idea to note this in the config doc string as
> > well,
> > > > so users don't try to use it as if it was a
> > > > literal replacement of the *deprecated
> > > > default.windowed.key.serde.inner* config.
> > > > What do you think?
> > > > (To clarify, I'm suggesting we check inside the KafkaStreams
> > constructor
> > > > and throw if this config is set, but this
> > > > last bit doesn't need to go into the KIP as it's an implementation
> > detail)
> > > >
> > > >
> > > > Once you've addressed the above and cleaned the KIP up a bit, I'm +1
> > -- the
> > > > changes you propose make sense to me.
> > > >
> > > > Cheers,
> > > > Sophie
> > > >
> > > > On Mon, Mar 22, 2021 at 3:28 AM Sagar <sa...@gmail.com>
> > wrote:
> > > >
> > > > > Hi All,
> > > > >
> > > > > I would like to start a discussion on the following KIP:
> > > > >
> > > > >
> > > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=177047930
> > > > >
> > > > > Thanks!
> > > > > Sagar.
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-725: Streamlining configurations for TimeWindowedDeserializer.

Posted by Sagar <sa...@gmail.com>.

Thanks John!

Yeah I think window.inner.class.deserializer sounds good. Your thoughts
@Sophie?

Thanks!
Sagar.


On Fri, Apr 2, 2021 at 5:23 AM John Roesler <vv...@apache.org> wrote:

> Hi Sagar,
>
> I think this is a good proposal.
>
> The only change I might recommend is to change the config to more closely
> align with the other one, for example: “window.inner.class.deserializer”
>
> But it’s very minor; I leave it to your judgement.
>
> Thanks,
> John
>
> On Fri, Mar 26, 2021, at 03:36, Sagar wrote:
> > Hi Sophie,
> >
> > Thanks for the feedback! I have updated the KIP inline with whatever you
> > suggested.
> >
> > Regarding point 5, I have added the note as it makes sense to not set the
> > config via the KafkaStreams app.
> >
> > Thanks!
> > Sagar.
> >
> >
> > On Wed, Mar 24, 2021 at 7:52 AM Sophie Blee-Goldman
> > <so...@confluent.io.invalid> wrote:
> >
> > > Hey Sagar,
> > >
> > > Thanks for the KIP! The overall proposal looks good to me, but I had
> some
> > > miscellaneous notes:
> > >
> > > 1) Some general KIP-writing advice:
> > >     - You don't need to list any implementation details, only public
> > > interfaces that are being added, modified, or
> > >       deprecated. It's better to describe your changes in words, or
> > > occasionally psuedo-code for more complicated
> > >       changes or algorithms. The KIP is a public contract, so you
> generally
> > > want to agree upon the high-level
> > >       proposal and avoid getting locked in to specific code which you
> may
> > > end up wanting to change once you
> > >       start on the PR.
> > >       In this KIP, you can remove the code block under
> > > *TimeWindowedDeserializer
> > > *and just describe the desired
> > >       semantics: eg that you should only use the constructor within
> > > Streams, the configs within the console consumer,
> > >       or either for a plain consumer client provided they match.
> > >       The code block under *StreamsConfig *however is a good example of
> > > what should be in a KIP. Only one nit:
> > >       in the doc string, avoid saying "windowed key" and just say
> "windowed
> > > record" or something like that.
> > >     - The *Motivation* section should be focused on any background or
> > > additional context that's necessary to
> > >       understand the KIP, as well as the actual motivation for the
> change
> > > being proposed. So here, everything under
> > >       the second bullet which begins with "The KIP also introduces
> > > changes..." should be cut from that section and
> > >       discussed elsewhere.
> > >     - The *Proposed Changes* and *Public Interfaces* sections have a
> lot of
> > > overlap and repeated content. To be
> > >       honest I personally have struggled with deciding which section to
> > > include something in, but a good rule of thumb
> > >       is to describe the actual changes in the *Proposed Changes*
> section,
> > > and then use the *Public Interfaces* section
> > >       to list any new or modified public APIs. In this case, I would
> move
> > > everything to the *Proposed Changes* section
> > >       except for the code block with the deprecated config(s) and the
> new
> > > config + doc string.
> > >
> > > 2) Can you make it clear that both *default.windowed.key.serde.inner*
> and
> > > *default.windowed.key.serde.inner *
> > > are being deprecated, and we're adding a new config called
> > > *windowed.deserializer.inner.class*, not literally
> > > renaming the existing *default.windowed.key.serde.inner* config? I
> think
> > > you're hinting at this in the
> > > *Compatibility, Deprecation, and Migration* section, but elsewhere in
> the
> > > KIP it's implied that we'll be replacing
> > > existing config which would break any applications that currently rely
> on
> > > it. Please update the *Public Interfaces*
> > > and *Proposed Changes* sections to clarify that both of these configs
> will
> > > be deprecated.
> > >
> > > 3) At the end of this section you raise a question that I don't think I
> > > quite understand, can you elaborate on this:
> > >
> > > We can maybe enforce the removal of the deprecated configs and then
> enforce
> > > > users?
> > > >
> > > Note: you only need to worry about deprecating these configs in the
> current
> > > KIP. Once deprecated we just need to
> > > give users enough time to migrate over to the new API, and then we can
> > > remove them in the next major version.
> > > The important thing for the KIP itself is just to make sure the change
> is
> > > visible to users and provides a clear
> > > migration path.
> > >
> > > 4) For the Console Consumer you say
> > >
> > > It would be mandatory to pass windowed.deserializer.inner.class and
> > > > window.size.ms config. <Need to check how to do this>
> > > >
> > >
> > > As I said above, you don't need to worry about putting how you'll
> implement
> > > this in the KIP document.  But to answer
> > > your implicit question, I actually don't think you need to do anything
> > > special for the  console consumer -- the
> > > TimeWindowedDeserializer itself will verify that both its parameters
> have
> > > been set and throw an exception if not.
> > >
> > > 5) One last small suggestion: I think we should throw an exception if a
> > > user has tried to use the new
> > > *windowed.deserializer.inner.class *config in their Kafka Streams
> > > application, since it's intended for use only
> > > with/for the plain consumer client. If you agree, this is something
> that
> > > should be noted in the KIP somewhere.
> > > It may also be a good idea to note this in the config doc string as
> well,
> > > so users don't try to use it as if it was a
> > > literal replacement of the *deprecated
> > > default.windowed.key.serde.inner* config.
> > > What do you think?
> > > (To clarify, I'm suggesting we check inside the KafkaStreams
> constructor
> > > and throw if this config is set, but this
> > > last bit doesn't need to go into the KIP as it's an implementation
> detail)
> > >
> > >
> > > Once you've addressed the above and cleaned the KIP up a bit, I'm +1
> -- the
> > > changes you propose make sense to me.
> > >
> > > Cheers,
> > > Sophie
> > >
> > > On Mon, Mar 22, 2021 at 3:28 AM Sagar <sa...@gmail.com>
> wrote:
> > >
> > > > Hi All,
> > > >
> > > > I would like to start a discussion on the following KIP:
> > > >
> > > >
> > >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=177047930
> > > >
> > > > Thanks!
> > > > Sagar.
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-725: Streamlining configurations for TimeWindowedDeserializer.

Posted by John Roesler <vv...@apache.org>.

Hi Sagar,

I think this is a good proposal.

The only change I might recommend is to change the config to more closely align with the other one, for example: “window.inner.class.deserializer”

But it’s very minor; I leave it to your judgement. 

Thanks,
John

On Fri, Mar 26, 2021, at 03:36, Sagar wrote:
> Hi Sophie,
> 
> Thanks for the feedback! I have updated the KIP inline with whatever you
> suggested.
> 
> Regarding point 5, I have added the note as it makes sense to not set the
> config via the KafkaStreams app.
> 
> Thanks!
> Sagar.
> 
> 
> On Wed, Mar 24, 2021 at 7:52 AM Sophie Blee-Goldman
> <so...@confluent.io.invalid> wrote:
> 
> > Hey Sagar,
> >
> > Thanks for the KIP! The overall proposal looks good to me, but I had some
> > miscellaneous notes:
> >
> > 1) Some general KIP-writing advice:
> >     - You don't need to list any implementation details, only public
> > interfaces that are being added, modified, or
> >       deprecated. It's better to describe your changes in words, or
> > occasionally psuedo-code for more complicated
> >       changes or algorithms. The KIP is a public contract, so you generally
> > want to agree upon the high-level
> >       proposal and avoid getting locked in to specific code which you may
> > end up wanting to change once you
> >       start on the PR.
> >       In this KIP, you can remove the code block under
> > *TimeWindowedDeserializer
> > *and just describe the desired
> >       semantics: eg that you should only use the constructor within
> > Streams, the configs within the console consumer,
> >       or either for a plain consumer client provided they match.
> >       The code block under *StreamsConfig *however is a good example of
> > what should be in a KIP. Only one nit:
> >       in the doc string, avoid saying "windowed key" and just say "windowed
> > record" or something like that.
> >     - The *Motivation* section should be focused on any background or
> > additional context that's necessary to
> >       understand the KIP, as well as the actual motivation for the change
> > being proposed. So here, everything under
> >       the second bullet which begins with "The KIP also introduces
> > changes..." should be cut from that section and
> >       discussed elsewhere.
> >     - The *Proposed Changes* and *Public Interfaces* sections have a lot of
> > overlap and repeated content. To be
> >       honest I personally have struggled with deciding which section to
> > include something in, but a good rule of thumb
> >       is to describe the actual changes in the *Proposed Changes* section,
> > and then use the *Public Interfaces* section
> >       to list any new or modified public APIs. In this case, I would move
> > everything to the *Proposed Changes* section
> >       except for the code block with the deprecated config(s) and the new
> > config + doc string.
> >
> > 2) Can you make it clear that both *default.windowed.key.serde.inner* and
> > *default.windowed.key.serde.inner *
> > are being deprecated, and we're adding a new config called
> > *windowed.deserializer.inner.class*, not literally
> > renaming the existing *default.windowed.key.serde.inner* config? I think
> > you're hinting at this in the
> > *Compatibility, Deprecation, and Migration* section, but elsewhere in the
> > KIP it's implied that we'll be replacing
> > existing config which would break any applications that currently rely on
> > it. Please update the *Public Interfaces*
> > and *Proposed Changes* sections to clarify that both of these configs will
> > be deprecated.
> >
> > 3) At the end of this section you raise a question that I don't think I
> > quite understand, can you elaborate on this:
> >
> > We can maybe enforce the removal of the deprecated configs and then enforce
> > > users?
> > >
> > Note: you only need to worry about deprecating these configs in the current
> > KIP. Once deprecated we just need to
> > give users enough time to migrate over to the new API, and then we can
> > remove them in the next major version.
> > The important thing for the KIP itself is just to make sure the change is
> > visible to users and provides a clear
> > migration path.
> >
> > 4) For the Console Consumer you say
> >
> > It would be mandatory to pass windowed.deserializer.inner.class and
> > > window.size.ms config. <Need to check how to do this>
> > >
> >
> > As I said above, you don't need to worry about putting how you'll implement
> > this in the KIP document.  But to answer
> > your implicit question, I actually don't think you need to do anything
> > special for the  console consumer -- the
> > TimeWindowedDeserializer itself will verify that both its parameters have
> > been set and throw an exception if not.
> >
> > 5) One last small suggestion: I think we should throw an exception if a
> > user has tried to use the new
> > *windowed.deserializer.inner.class *config in their Kafka Streams
> > application, since it's intended for use only
> > with/for the plain consumer client. If you agree, this is something that
> > should be noted in the KIP somewhere.
> > It may also be a good idea to note this in the config doc string as well,
> > so users don't try to use it as if it was a
> > literal replacement of the *deprecated
> > default.windowed.key.serde.inner* config.
> > What do you think?
> > (To clarify, I'm suggesting we check inside the KafkaStreams constructor
> > and throw if this config is set, but this
> > last bit doesn't need to go into the KIP as it's an implementation detail)
> >
> >
> > Once you've addressed the above and cleaned the KIP up a bit, I'm +1 -- the
> > changes you propose make sense to me.
> >
> > Cheers,
> > Sophie
> >
> > On Mon, Mar 22, 2021 at 3:28 AM Sagar <sa...@gmail.com> wrote:
> >
> > > Hi All,
> > >
> > > I would like to start a discussion on the following KIP:
> > >
> > >
> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=177047930
> > >
> > > Thanks!
> > > Sagar.
> > >
> >
>

Re: [DISCUSS] KIP-725: Streamlining configurations for TimeWindowedDeserializer.

Posted by Sagar <sa...@gmail.com>.

Hi Sophie,

Thanks for the feedback! I have updated the KIP inline with whatever you
suggested.

Regarding point 5, I have added the note as it makes sense to not set the
config via the KafkaStreams app.

Thanks!
Sagar.


On Wed, Mar 24, 2021 at 7:52 AM Sophie Blee-Goldman
<so...@confluent.io.invalid> wrote:

> Hey Sagar,
>
> Thanks for the KIP! The overall proposal looks good to me, but I had some
> miscellaneous notes:
>
> 1) Some general KIP-writing advice:
>     - You don't need to list any implementation details, only public
> interfaces that are being added, modified, or
>       deprecated. It's better to describe your changes in words, or
> occasionally psuedo-code for more complicated
>       changes or algorithms. The KIP is a public contract, so you generally
> want to agree upon the high-level
>       proposal and avoid getting locked in to specific code which you may
> end up wanting to change once you
>       start on the PR.
>       In this KIP, you can remove the code block under
> *TimeWindowedDeserializer
> *and just describe the desired
>       semantics: eg that you should only use the constructor within
> Streams, the configs within the console consumer,
>       or either for a plain consumer client provided they match.
>       The code block under *StreamsConfig *however is a good example of
> what should be in a KIP. Only one nit:
>       in the doc string, avoid saying "windowed key" and just say "windowed
> record" or something like that.
>     - The *Motivation* section should be focused on any background or
> additional context that's necessary to
>       understand the KIP, as well as the actual motivation for the change
> being proposed. So here, everything under
>       the second bullet which begins with "The KIP also introduces
> changes..." should be cut from that section and
>       discussed elsewhere.
>     - The *Proposed Changes* and *Public Interfaces* sections have a lot of
> overlap and repeated content. To be
>       honest I personally have struggled with deciding which section to
> include something in, but a good rule of thumb
>       is to describe the actual changes in the *Proposed Changes* section,
> and then use the *Public Interfaces* section
>       to list any new or modified public APIs. In this case, I would move
> everything to the *Proposed Changes* section
>       except for the code block with the deprecated config(s) and the new
> config + doc string.
>
> 2) Can you make it clear that both *default.windowed.key.serde.inner* and
> *default.windowed.key.serde.inner *
> are being deprecated, and we're adding a new config called
> *windowed.deserializer.inner.class*, not literally
> renaming the existing *default.windowed.key.serde.inner* config? I think
> you're hinting at this in the
> *Compatibility, Deprecation, and Migration* section, but elsewhere in the
> KIP it's implied that we'll be replacing
> existing config which would break any applications that currently rely on
> it. Please update the *Public Interfaces*
> and *Proposed Changes* sections to clarify that both of these configs will
> be deprecated.
>
> 3) At the end of this section you raise a question that I don't think I
> quite understand, can you elaborate on this:
>
> We can maybe enforce the removal of the deprecated configs and then enforce
> > users?
> >
> Note: you only need to worry about deprecating these configs in the current
> KIP. Once deprecated we just need to
> give users enough time to migrate over to the new API, and then we can
> remove them in the next major version.
> The important thing for the KIP itself is just to make sure the change is
> visible to users and provides a clear
> migration path.
>
> 4) For the Console Consumer you say
>
> It would be mandatory to pass windowed.deserializer.inner.class and
> > window.size.ms config. <Need to check how to do this>
> >
>
> As I said above, you don't need to worry about putting how you'll implement
> this in the KIP document.  But to answer
> your implicit question, I actually don't think you need to do anything
> special for the  console consumer -- the
> TimeWindowedDeserializer itself will verify that both its parameters have
> been set and throw an exception if not.
>
> 5) One last small suggestion: I think we should throw an exception if a
> user has tried to use the new
> *windowed.deserializer.inner.class *config in their Kafka Streams
> application, since it's intended for use only
> with/for the plain consumer client. If you agree, this is something that
> should be noted in the KIP somewhere.
> It may also be a good idea to note this in the config doc string as well,
> so users don't try to use it as if it was a
> literal replacement of the *deprecated
> default.windowed.key.serde.inner* config.
> What do you think?
> (To clarify, I'm suggesting we check inside the KafkaStreams constructor
> and throw if this config is set, but this
> last bit doesn't need to go into the KIP as it's an implementation detail)
>
>
> Once you've addressed the above and cleaned the KIP up a bit, I'm +1 -- the
> changes you propose make sense to me.
>
> Cheers,
> Sophie
>
> On Mon, Mar 22, 2021 at 3:28 AM Sagar <sa...@gmail.com> wrote:
>
> > Hi All,
> >
> > I would like to start a discussion on the following KIP:
> >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=177047930
> >
> > Thanks!
> > Sagar.
> >
>

Re: [DISCUSS] KIP-725: Streamlining configurations for TimeWindowedDeserializer.

Posted by Sophie Blee-Goldman <so...@confluent.io.INVALID>.

Hey Sagar,

Thanks for the KIP! The overall proposal looks good to me, but I had some
miscellaneous notes:

1) Some general KIP-writing advice:
    - You don't need to list any implementation details, only public
interfaces that are being added, modified, or
      deprecated. It's better to describe your changes in words, or
occasionally psuedo-code for more complicated
      changes or algorithms. The KIP is a public contract, so you generally
want to agree upon the high-level
      proposal and avoid getting locked in to specific code which you may
end up wanting to change once you
      start on the PR.
      In this KIP, you can remove the code block under
*TimeWindowedDeserializer
*and just describe the desired
      semantics: eg that you should only use the constructor within
Streams, the configs within the console consumer,
      or either for a plain consumer client provided they match.
      The code block under *StreamsConfig *however is a good example of
what should be in a KIP. Only one nit:
      in the doc string, avoid saying "windowed key" and just say "windowed
record" or something like that.
    - The *Motivation* section should be focused on any background or
additional context that's necessary to
      understand the KIP, as well as the actual motivation for the change
being proposed. So here, everything under
      the second bullet which begins with "The KIP also introduces
changes..." should be cut from that section and
      discussed elsewhere.
    - The *Proposed Changes* and *Public Interfaces* sections have a lot of
overlap and repeated content. To be
      honest I personally have struggled with deciding which section to
include something in, but a good rule of thumb
      is to describe the actual changes in the *Proposed Changes* section,
and then use the *Public Interfaces* section
      to list any new or modified public APIs. In this case, I would move
everything to the *Proposed Changes* section
      except for the code block with the deprecated config(s) and the new
config + doc string.

2) Can you make it clear that both *default.windowed.key.serde.inner* and
*default.windowed.key.serde.inner *
are being deprecated, and we're adding a new config called
*windowed.deserializer.inner.class*, not literally
renaming the existing *default.windowed.key.serde.inner* config? I think
you're hinting at this in the
*Compatibility, Deprecation, and Migration* section, but elsewhere in the
KIP it's implied that we'll be replacing
existing config which would break any applications that currently rely on
it. Please update the *Public Interfaces*
and *Proposed Changes* sections to clarify that both of these configs will
be deprecated.

3) At the end of this section you raise a question that I don't think I
quite understand, can you elaborate on this:

We can maybe enforce the removal of the deprecated configs and then enforce
> users?
>
Note: you only need to worry about deprecating these configs in the current
KIP. Once deprecated we just need to
give users enough time to migrate over to the new API, and then we can
remove them in the next major version.
The important thing for the KIP itself is just to make sure the change is
visible to users and provides a clear
migration path.

4) For the Console Consumer you say

It would be mandatory to pass windowed.deserializer.inner.class and
> window.size.ms config. <Need to check how to do this>
>

As I said above, you don't need to worry about putting how you'll implement
this in the KIP document.  But to answer
your implicit question, I actually don't think you need to do anything
special for the  console consumer -- the
TimeWindowedDeserializer itself will verify that both its parameters have
been set and throw an exception if not.

5) One last small suggestion: I think we should throw an exception if a
user has tried to use the new
*windowed.deserializer.inner.class *config in their Kafka Streams
application, since it's intended for use only
with/for the plain consumer client. If you agree, this is something that
should be noted in the KIP somewhere.
It may also be a good idea to note this in the config doc string as well,
so users don't try to use it as if it was a
literal replacement of the *deprecated
default.windowed.key.serde.inner* config.
What do you think?
(To clarify, I'm suggesting we check inside the KafkaStreams constructor
and throw if this config is set, but this
last bit doesn't need to go into the KIP as it's an implementation detail)

Once you've addressed the above and cleaned the KIP up a bit, I'm +1 -- the
changes you propose make sense to me.

Cheers,
Sophie

On Mon, Mar 22, 2021 at 3:28 AM Sagar <sa...@gmail.com> wrote:

> Hi All,
>
> I would like to start a discussion on the following KIP:
>
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=177047930
>
> Thanks!
> Sagar.
>