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