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