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