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