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