Thanks Matthias -- sorry I forgot to get back to you! Thanks for kicking off the voting thread on this
On Thu, Mar 28, 2024 at 3:58 AM Matthias J. Sax <mj...@apache.org> wrote: > Thanks. I think you can start a VOTE. > > -Matthias > > > On 3/20/24 9:24 PM, Lucia Cerchie wrote: > > thanks Sophie, I've made the updates, would appreciate one more look > before > > submission > > > > On Wed, Mar 20, 2024 at 8:36 AM Sophie Blee-Goldman < > sop...@responsive.dev> > > wrote: > > > >> A few minor notes on the KIP but otherwise I think you can go ahead and > >> call for a vote! > >> > >> 1. Need to update the Motivation section to say "console clients" or > >> "console consumer/producer" instead of " plain consumer client" > >> 2. Remove the first paragraph under "Public Interfaces" (ie the > KIP-writing > >> instructions) and also list the new config definitions here. You can > just > >> add a code snippet for each class (TimeWindowedDe/Serializer) with the > >> actual variable definition you'll be adding. Maybe also add a code > snippet > >> for StreamsConfig with the @Deprecated annotation added to the two > configs > >> we're deprecating > >> 3. nit: remove the "questions" under "Compatibility, Deprecation, and > >> Migration Plan", ie the stuff from the KIP-writing template. Just makes > it > >> easier to read > >> 4. In "Test Plan" we should also have a unit test to make sure the new " > >> windowed.inner.de/serializer.class" takes preference in the case that > both > >> it and the old "windowed.inner.serde.class" config are both specified > >> > >> Also, this is more of a question than a suggestion, but is the KIP title > >> perhaps a bit misleading in that people might assume these configs will > no > >> longer be available for use at all? What do people think about changing > it > >> to something like > >> > >> *Move `window.size.ms <http://window.size.ms>` and > >> `windowed.inner.serde.class` from `StreamsConfig` to > >> TimeWindowedDe/Serializer class* > >> > >> A bit long/clunky but at least it gets the point across about where > folks > >> can find these configs going forward. > >> > >> On Mon, Mar 18, 2024 at 6:26 PM Lucia Cerchie > >> <lcerc...@confluent.io.invalid> > >> wrote: > >> > >>> Thanks for the discussion! > >>> > >>> I've updated the KIP here with what I believe are the relevant pieces, > >>> please let me know if anything is missing: > >>> > >> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=290982804 > >>> > >>> On Sun, Mar 17, 2024 at 7:09 PM Sophie Blee-Goldman < > >> sop...@responsive.dev > >>>> > >>> wrote: > >>> > >>>> Sounds good! > >>>> > >>>> @Lucia when you have a moment can you update the KIP with > >>>> the new proposal, including the details that Matthias pointed > >>>> out in his last response? After that's done I think you can go > >>>> ahead and call for a vote whenever you're ready! > >>>> > >>>> On Sat, Mar 16, 2024 at 7:35 PM Matthias J. Sax <mj...@apache.org> > >>> wrote: > >>>> > >>>>> Thanks for the summary. Sounds right to me. That is what I would > >>> propose. > >>>>> > >>>>> As you pointed out, we of course still need to support the current > >>>>> confis, and we should log a warning when in use (even if the new one > >> is > >>>>> in use IMHO) -- but that's more an implementation detail. > >>>>> > >>>>> I agree that the new config should take preference in case both are > >>>>> specified. This should be pointed out in the KIP, as it's an > >> important > >>>>> contract the user needs to understand. > >>>>> > >>>>> > >>>>> -Matthias > >>>>> > >>>>> On 3/14/24 6:18 PM, Sophie Blee-Goldman wrote: > >>>>>>> > >>>>>>> Should we change it do `.serializer` and `.deserialize`? > >>>>>> > >>>>>> That's a good point -- if we're going to split this up by defining > >>> the > >>>>>> config > >>>>>> in both the TimeWindowedSerializer and TimeWindowedDeserializer, > >>>>>> then it makes perfect sense to go a step further and actually > >> define > >>>>>> only the relevant de/serializer class instead of the full serde > >>>>>> > >>>>>> Just to put this all together, it sounds like the proposal is to: > >>>>>> > >>>>>> 1) Deprecate both these configs where they appear in StreamsConfig > >>>>>> (as per the original plan in the KIP, just reiterating it here) > >>>>>> > >>>>>> 2) Don't "define" either config in any specific client's Config > >>> class, > >>>>>> but just define a String variable with the config name in the > >>> relevant > >>>>>> de/serializer class, and maybe point people to it in the docs > >>> somewhere > >>>>>> > >>>>>> 3) We would add three new public String variables for three > >> different > >>>>>> configs across two classes, specifically: > >>>>>> > >>>>>> In TimeWindowedSerializer: > >>>>>> - define a constant for "windowed.inner.serializer.class" > >>>>>> In TimeWindowedDeserializer: > >>>>>> - define a constant for "windowed.inner.deserializer.class" > >>>>>> - define a constant for "window.size.ms" > >>>>>> > >>>>>> 4) Lastly, we would update the windowed de/serializer > >> implementations > >>>>>> to check for the new configs (ie " > >> windowed.inner.de/serializer.class > >>> ") > >>>>>> and use the provided de/serializer class, if one was given. If the > >>> new > >>>>>> configs are not present, they would fall back to the > >> original/current > >>>>>> logic (ie that based on the old "windowed.inner.serde.class" > >> config) > >>>>>> > >>>>>> I think that's everything. Does this sound about right for where we > >>>> want > >>>>>> to go with these configs? > >>>>>> > >>>>>> On Thu, Mar 14, 2024 at 4:58 PM Matthias J. Sax <mj...@apache.org> > >>>>> wrote: > >>>>>> > >>>>>>>>> By "don't add them" do you just mean we would not have any > >> actual > >>>>>>>>> variables defined anywhere for these configs (eg WINDOW_SIZE_MS) > >>>>>>>>> and simply document -- somewhere -- that one can use the string > >>>>>>>>> "window.size.ms" when configuring a command-line client with a > >>>>>>>>> windowed serde? > >>>>>>> > >>>>>>> Yes. That's the idea. > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>>> I assume you aren't proposing > >>>>>>>>> to remove the ability to use and understand this config from the > >>>>>>>>> implementations themselves, but correct me if that's wrong. > >>>>>>> > >>>>>>> No, that would effectively break what we fixed with the original > >> KIP > >>>> :) > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>>>> Are there any other configs in similar situations that we could > >>> look > >>>>>>>>> to for precedent? > >>>>>>> > >>>>>>> Not aware of any others, either. > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>>>> If these are truly the first/only of their kind, I would vote to > >>>> just > >>>>>>> stick > >>>>>>>>> them in the appropriate class. As for which class to put them > >> in, > >>> I > >>>>>>>>> think I'm convinced that "window.size.ms" should only go in the > >>>>>>>>> TimeWindowedDeserializer rather than sticking them both in the > >>>>>>>>> TimeWindowedSerde as I originally suggested. However, I would > >>>>>>>>> even go a step further and not place the > >>> "inner.window.class.serde" > >>>>>>>>> in the TimeWindowedSerde class either. To me, it actually makes > >>>>>>>>> the most sense to define it in both the TimeWindowedSerializer > >>>>>>>>> and the TimeWindowedDeserializer. > >>>>>>> > >>>>>>> Not sure either. What you propose is fine with me. However, I am > >>>>>>> wondering about the config names... Why is it `serde` for this > >> case? > >>>>>>> Should we change it do `.serializer` and `.deserialize`? > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> -Matthias > >>>>>>> > >>>>>>> > >>>>>>> On 3/13/24 8:19 PM, Sophie Blee-Goldman wrote: > >>>>>>>> By "don't add them" do you just mean we would not have any actual > >>>>>>>> variables defined anywhere for these configs (eg WINDOW_SIZE_MS) > >>>>>>>> and simply document -- somewhere -- that one can use the string > >>>>>>>> "window.size.ms" when configuring a command-line client with a > >>>>>>>> windowed serde? Or something else? I assume you aren't proposing > >>>>>>>> to remove the ability to use and understand this config from the > >>>>>>>> implementations themselves, but correct me if that's wrong. > >>>>>>>> > >>>>>>>> Are there any other configs in similar situations that we could > >>> look > >>>>>>>> to for precedent? I personally am not aware of any but by > >>> definition > >>>>>>>> I suppose these would be hard to discover unless you were > >> actively > >>>>>>>> looking for them, so I'm wondering if there might be other > >> "shadow > >>>>>>>> configs" elsewhere in the code base. > >>>>>>>> > >>>>>>>> If these are truly the first/only of their kind, I would vote to > >>> just > >>>>>>> stick > >>>>>>>> them in the appropriate class. As for which class to put them > >> in, I > >>>>>>>> think I'm convinced that "window.size.ms" should only go in the > >>>>>>>> TimeWindowedDeserializer rather than sticking them both in the > >>>>>>>> TimeWindowedSerde as I originally suggested. However, I would > >>>>>>>> even go a step further and not place the > >> "inner.window.class.serde" > >>>>>>>> in the TimeWindowedSerde class either. To me, it actually makes > >>>>>>>> the most sense to define it in both the TimeWindowedSerializer > >>>>>>>> and the TimeWindowedDeserializer. > >>>>>>>> > >>>>>>>> The reason being that, as discussed above, the only use case for > >>>>>>>> these configs would be in the console consumer/producer which > >>>>>>>> only uses the Serializer or Deserializer, and would never > >> actually > >>>>>>>> be used by/in Streams where we use the Serde version. And while > >>>>>>>> defining the "inner.window.class.serde" in two places might seem > >>>>>>>> redundant, this would mean that all the configs needed to > >> properly > >>>>>>>> configure the specific class being used by the particular kind of > >>>>>>>> consumer client -- that is, Deserializer for a console consumer > >> and > >>>>>>>> Serializer for a console producer -- would be located in that > >> exact > >>>>>>>> class. I assume this would make them much easier to discover > >>>>>>>> and be used than having to search for configs defined in classes > >>>>>>>> you don't even need for the console client, like the Serde form > >>>>>>>> > >>>>>>>> Just my two cents -- happy to hear other opinions on this > >>>>>>>> > >>>>>>>> On Mon, Mar 11, 2024 at 6:58 PM Matthias J. Sax < > >> mj...@apache.org> > >>>>>>> wrote: > >>>>>>>> > >>>>>>>>> Yes, it's used inside `TimeWindowedSerializer` and actually also > >>>>> inside > >>>>>>>>> `TimeWindowDeserializer`. > >>>>>>>>> > >>>>>>>>> However, it does IMHO not change that we should remove it from > >>>>>>>>> `StreamsConfig` because both configs are not intended to be used > >>> in > >>>>> Java > >>>>>>>>> code... If one writes Java code, they should use > >>>>>>>>> > >>>>>>>>> new TimeWindowedSerializer(Serializer) > >>>>>>>>> new TimeWindowDeserializer(Deserializer, Long) > >>>>>>>>> new TimeWindowSerdes(Serde, Long) > >>>>>>>>> > >>>>>>>>> and thus they don't need either config. > >>>>>>>>> > >>>>>>>>> The configs are only needed for command line tool, that create > >> the > >>>>>>>>> (de)serializer via reflection using the default constructor. > >>>>>>>>> > >>>>>>>>> Does this make sense? > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> The only open question is really, if and where to add them... > >>>> Strictly > >>>>>>>>> speaking, we don't need either config as public variable as > >> nobody > >>>>>>>>> should use them in Java code. To me, it just feels right/better > >> do > >>>>> make > >>>>>>>>> them public for documentation purpose that these configs exists? > >>>>>>>>> > >>>>>>>>> `inner.window.class.serde` has "serde" in it's name, so we could > >>> add > >>>>> it > >>>>>>>>> to `TimeWindowSerdes`? For `window.size.ms`, it's only used by > >>> the > >>>>>>>>> deserialize to maybe add it there? Just some ideas. -- Or we > >>>> sidestep > >>>>>>>>> this question and just don't add them; also fine with me. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> -Matthias > >>>>>>>>> > >>>>>>>>> On 3/11/24 10:53 AM, Lucia Cerchie wrote: > >>>>>>>>>> PS-- I was re-reading the PR that originated this discussion > >> and > >>>>>>> realized > >>>>>>>>>> that `window.inner.serde.class` is used here > >>>>>>>>>> < > >>>>>>>>> > >>>>>>> > >>>>> > >>>> > >>> > >> > https://github.com/a0x8o/kafka/blob/master/streams/src/main/java/org/apache/kafka/streams/kstream/TimeWindowedSerializer.java#L44 > >>>>>>>>>> > >>>>>>>>>> in KStreams. This goes against removing it, yes? > >>>>>>>>>> > >>>>>>>>>> On Mon, Mar 11, 2024 at 10:40 AM Lucia Cerchie < > >>>>> lcerc...@confluent.io> > >>>>>>>>>> wrote: > >>>>>>>>>> > >>>>>>>>>>> Sophie, I'll add a paragraph about removing > >>>>>>> `windowed.inner.serde.class` > >>>>>>>>>>> to the KIP. I'll also add putting it in the > >> `TimeWindowedSerde` > >>>>> class > >>>>>>>>> with > >>>>>>>>>>> some add'tl guidance on the docs addition. > >>>>>>>>>>> > >>>>>>>>>>> Also, I double-checked setting window.size.ms on the client > >> and > >>>> it > >>>>>>>>>>> doesn't throw an error at all, in response to Matthias's > >>> question. > >>>>>>>>> Changing > >>>>>>>>>>> the KIP in response to that. > >>>>>>>>>>> > >>>>>>>>>>> On Sun, Mar 10, 2024 at 6:04 PM Sophie Blee-Goldman < > >>>>>>>>> sop...@responsive.dev> > >>>>>>>>>>> wrote: > >>>>>>>>>>> > >>>>>>>>>>>> Thanks for responding Matthias -- you got there first, but I > >>> was > >>>>>>> going > >>>>>>>>> to > >>>>>>>>>>>> say exactly the same thing as in your most reply. In other > >>>> words, I > >>>>>>> see > >>>>>>>>>>>> the > >>>>>>>>>>>> `windowed.inner.serde.class` as being in the same boat as > >> the ` > >>>>>>>>>>>> window.size.ms` config, so whatever we do with one we should > >>> do > >>>>> for > >>>>>>>>> the > >>>>>>>>>>>> other. > >>>>>>>>>>>> > >>>>>>>>>>>> I do agree with removing these from StreamsConfig, but > >> defining > >>>>> them > >>>>>>> in > >>>>>>>>>>>> ConsumerConfig feels weird as well. There's really no great > >>>> answer > >>>>>>>>> here. > >>>>>>>>>>>> > >>>>>>>>>>>> My only concern about adding it to the corresponding > >>>>>>>>>>>> serde/serializer/deserializer class is that it might be > >>> difficult > >>>>> for > >>>>>>>>>>>> people to find them. I generally assume that people tend not > >> to > >>>>> look > >>>>>>> at > >>>>>>>>>>>> the > >>>>>>>>>>>> serde/serializer/deserializer classes/implementations. But > >>> maybe > >>>> in > >>>>>>>>> this > >>>>>>>>>>>> case, someone who actually needed these configs would be > >> likely > >>>> to > >>>>> be > >>>>>>>>>>>> motivated enough to find them by looking at the class? And > >> with > >>>>>>>>> sufficient > >>>>>>>>>>>> documentation, it's likely not a problem. So, I'm +1 on > >> putting > >>>> it > >>>>>>> into > >>>>>>>>>>>> the > >>>>>>>>>>>> TimeWindowedSerde class > >>>>>>>>>>>> > >>>>>>>>>>>> (I would personally stick them into the serde class, rather > >>> than > >>>>> the > >>>>>>>>>>>> serializer and/or deserializer, but I could be convinced > >> either > >>>>> way) > >>>>>>>>>>>> > >>>>>>>>>>>> On Fri, Mar 1, 2024 at 3:00 PM Matthias J. Sax < > >>> mj...@apache.org > >>>>> > >>>>>>>>> wrote: > >>>>>>>>>>>> > >>>>>>>>>>>>> One more thought after I did some more digging on the > >> related > >>>> PR. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Should we do the same thing for > >> `windowed.inner.serde.class`? > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> Both config belong to windowed serdes (which KS provide) but > >>> the > >>>>> KS > >>>>>>>>> code > >>>>>>>>>>>>> itself does never use them (and in fact, disallows to use > >> them > >>>> and > >>>>>>>>> would > >>>>>>>>>>>>> throw an error is used). Both are intended for plain > >> consumer > >>>> use > >>>>>>>>> cases > >>>>>>>>>>>>> for which the window serdes are used. > >>>>>>>>>>>>> > >>>>>>>>>>>>> The question to me is, should we add them back somewhere > >> else? > >>>> It > >>>>>>> does > >>>>>>>>>>>>> not really belong into `ConsumerConfig` either, but maybe we > >>>> could > >>>>>>> add > >>>>>>>>>>>>> them to the corresponding serde or (de)serialize classes? > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> -Matthias > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> On 2/21/24 2:41 PM, Matthias J. Sax wrote: > >>>>>>>>>>>>>> Thanks for the KIP. Sounds like a nice cleanup. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> window.size.ms is not a true KafkaStreams config, and > >>>> results > >>>>> in > >>>>>>>>> an > >>>>>>>>>>>>>>> error when set from a KStreams application > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> What error? > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Given that the configs is used by > >> `TimeWindowedDeserializer` > >>> I > >>>> am > >>>>>>>>>>>>>> wondering if we should additionally add > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> public class TimeWindowedDeserializer { > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> public static final String WINDOW_SIZE_MS_CONFIG = > >> " > >>>>>>>>>>>> window.size.ms > >>>>>>>>>>>>> "; > >>>>>>>>>>>>>> } > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> -Matthias > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> On 2/15/24 6:32 AM, Lucia Cerchie wrote: > >>>>>>>>>>>>>>> Hey everyone, > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> I'd like to discuss KIP-1020 > >>>>>>>>>>>>>>> < > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>> > >>>>>>> > >>>>> > >>>> > >>> > >> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=290982804 > >>>>>>>>>>>>>> , > >>>>>>>>>>>>>>> which would move to deprecate `window.size.ms` in > >>>>> `StreamsConfig` > >>>>>>>>>>>>> since ` > >>>>>>>>>>>>>>> window.size.ms` is a client config. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Thanks in advance! > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Lucia Cerchie > >>>>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> -- > >>>>>>>>>>> > >>>>>>>>>>> [image: Confluent] <https://www.confluent.io> > >>>>>>>>>>> Lucia Cerchie > >>>>>>>>>>> Developer Advocate > >>>>>>>>>>> Follow us: [image: Blog] > >>>>>>>>>>> < > >>>>>>>>> > >>>>>>> > >>>>> > >>>> > >>> > >> > https://www.confluent.io/blog?utm_source=footer&utm_medium=email&utm_campaign=ch.email-signature_type.community_content.blog > >>>>>>>>>> [image: > >>>>>>>>>>> Twitter] <https://twitter.com/ConfluentInc>[image: Slack] > >>>>>>>>>>> <https://slackpass.io/confluentcommunity>[image: YouTube] > >>>>>>>>>>> <https://youtube.com/confluent> > >>>>>>>>>>> > >>>>>>>>>>> [image: Try Confluent Cloud for Free] > >>>>>>>>>>> < > >>>>>>>>> > >>>>>>> > >>>>> > >>>> > >>> > >> > https://www.confluent.io/get-started?utm_campaign=tm.fm-apac_cd.inbound&utm_source=gmail&utm_medium=organic > >>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >>> > >>> -- > >>> > >>> [image: Confluent] <https://www.confluent.io> > >>> Lucia Cerchie > >>> Developer Advocate > >>> Follow us: [image: Blog] > >>> < > >>> > >> > https://www.confluent.io/blog?utm_source=footer&utm_medium=email&utm_campaign=ch.email-signature_type.community_content.blog > >>>> [image: > >>> Twitter] <https://twitter.com/ConfluentInc>[image: Slack] > >>> <https://slackpass.io/confluentcommunity>[image: YouTube] > >>> <https://youtube.com/confluent> > >>> > >>> [image: Try Confluent Cloud for Free] > >>> < > >>> > >> > https://www.confluent.io/get-started?utm_campaign=tm.fm-apac_cd.inbound&utm_source=gmail&utm_medium=organic > >>>> > >>> > >> > > > > >
