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