Thanks Sophie, Most of these changes should be there in my PR. I will wait for a couple of days into next week to see if concerns are raised.
Thanks! Sagar. On Sat, Apr 24, 2021 at 1:07 AM Sophie Blee-Goldman <sop...@confluent.io.invalid> wrote: > Thanks Sagar, these changes make sense to me. I don't think we need to call > for a > new vote unless there are any concerns raised, but I feel this is probably > not > too controversial. > > Thanks for the update > > On Fri, Apr 23, 2021 at 3:17 AM Sagar <sagarmeansoc...@gmail.com> wrote: > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
