Hi,
I think using ConfigException makes sense. But I am also fine with
SerdeConfigurationException. I think both are meaningful in this
situation where the former has the advantage that we do not need to
introduce a new exception.
Best,
Bruno
On 20.05.21 07:54, Guozhang Wang wrote:
Thanks Sophie. I think not piggy-backing on TopologyException makes sense.
It just occurs to me that today we already have similar situations even
with this config default to Bytes, that is the other
`DEFAULT_WINDOWED_KEY/VALUE_SERDE_INNER_CLASS` config, whose default is
actually null. Quickly checking the code here, I think we throw
StreamsException when they are found not defined during runtime, we
actually throw the `*ConfigException*`. So for consistency we could just
use that exception as well.
Guozhang
On Wed, May 19, 2021 at 3:24 PM Sophie Blee-Goldman
<sop...@confluent.io.invalid> wrote:
To be honest I'm not really a fan of reusing the TopologyException since it
feels like
a bit of a stretch from a user point of view to classify Serde
misconfiguration as a
topology issue.
I personally think a StreamsException would be acceptable, but I would also
propose
to introduce a new type of exception, something like
SerdeConfigurationException or
so. We certainly don't want to end up like the Producer API with its 500
different
exceptions. Luckily Streams is nowhere near that yet, in my opinion, and
problems
with Serde configuration are so common and well-defined that a dedicated
exception
feels very appropriate.
If there are any other instances in the codebase where we throw a
StreamsException
for a Serde-related issue, this could also be migrated to the new exception
type (not
necessarily all at once, but gradually after this KIP)
Thoughts?
On Wed, May 19, 2021 at 10:31 AM Guozhang Wang <wangg...@gmail.com> wrote:
Leah, thanks for the KIP.
It looks good to me overall, just following up on @br...@confluent.io
<br...@confluent.io> 's question about exception: what about using the
`TopologyException` class? I know that currently it is only thrown during
the topology parsing phase, not at the streams construction, but I feel
we
can extend its scope to cover both topology building and streams object
(i.e. taking the topology and the config) construction time as well since
part of the construction is again to re-write / augment the topology.
Guozhang
On Wed, May 19, 2021 at 8:44 AM Leah Thomas <ltho...@confluent.io.invalid
wrote:
Hi Sophie,
Thanks for catching that. These are existing methods inside of
`StreamsConfig` that will return null (the new default) instead of byte
array serde (the old default). Both `StreamsConfig` and
`defaultKeySerde`/`defaultValueSerde` are public, so I assume these
still
count as part of the public API. I updated the KIP to include this
information.
Bruno - I was planning on including a specific message with the streams
exception to indicate that either a serde needs to be passed in or a
default needs to be set. I'm open to doing something more specific,
perhaps something like a serde exception? WDYT? I was hoping that with
the
message the streams exception would still give enough information for
users
to debug the problem.
Still hoping for a short discussion (; but thanks for the input so far!
Leah
On Wed, May 19, 2021 at 3:00 AM Bruno Cadonna <cado...@apache.org>
wrote:
Hey Leah,
> what I think should be a small discussion
Dangerous words, indeed! It seems like they trigger something in
people
;-)
Jokes apart!
Did you consider throwing a more specific exception instead of a
StreamsException? Something that describes better the issue at hand.
Best,
Bruno
On 19.05.21 01:19, Sophie Blee-Goldman wrote:
what I think should be a small discussion
Dangerous words :P
I'm all for the proposal but I do have one question about something
in
the
KIP. You list two methods called
defaultKeySerde() and defaultValueSerde() but it's not clear to me
where
these are coming from. Are they
new APIs you propose to add in this KIP? Are they existing methods
in
the
public API which will now return
null, whereas they used to return the ByteArraySerde? If they're
not
a
public API then you can remove them
from the KIP, otherwise can you just update this section to clarify
what
class/file these belong to, etc?
-Sophie
On Tue, May 18, 2021 at 5:34 AM Leah Thomas
<ltho...@confluent.io.invalid
wrote:
Hi all,
I'd like to kick-off what I think should be a small discussion for
KIP-741:
Change default serde to be null.
The wiki is here:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-741%3A+Change+default+serde+to+be+null
Thanks,
Leah
--
-- Guozhang
