If there are no further notes, I'll go ahead and start the voting thread today.
Thanks, Leah On Thu, May 20, 2021 at 2:58 PM Leah Thomas <ltho...@confluent.io> wrote: > 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 >> > >>> >> > >> >> > > >> > > >> > >> >
