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