Thanks for finding that, Guozhang. Consistency seems like the best option to me as well, for the time being. I updated the KIP with that detail
On Thu, May 20, 2021 at 11:53 AM Sophie Blee-Goldman <sop...@confluent.io.invalid> wrote: > 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 > > >>> > > >> > > > > > > > > >
