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