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
