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 <sagarmeansoc...@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 <vvcep...@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 > > > <sop...@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 <sagarmeansoc...@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. > > > > > > > > > > > > > > >
