Thanks Guozhang, I forgot about ConfigException. I actually just reviewed two KIPs related to config-based Serdes, both of which use the ConfigException in this way: the ListSerde KIP, and the KIP to clean up the windowed inner class serde configuration.
For the sake of simplicity and keeping this KIP well-scoped, I would prefer to stick with the ConfigException for now, since this is consistent with how these very similar cases are handled at the moment. I would still stand by the idea of introducing a dedicated SerdeConfigurationException (or similar) but I think we can treat that as orthogonal, and maybe do a followup KIP at some point to convert all of these relevant cases over to a new Serde-specific exception On Thu, May 20, 2021 at 3:33 AM Bruno Cadonna <cado...@apache.org> wrote: > 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 > >>> > >> > > > > >
