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