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 >
