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
