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