Sorry for the delay in reply here. My personal life is a bit interesting at the moment so keeping up with email seems more and more impossible. :)
That all makes sense to me! Thanks for entertaining the questions! On Thu, Dec 6, 2018 at 1:01 PM Kamal Chandraprakash < kamal.chandraprak...@gmail.com> wrote: > Matt, > I agree with Matthias on not to altering the serializer as it's used by > multiple components. > > Matthias, > > - the proposed method accepts a `ProducerRecord` -- it might be good to > explain why this cannot be done in a type safe way (ie, missing generics) > > To accept different types of records from multiple topologies, I have to > define the ProducerRecord without generics. > > - `AlwaysProductionExceptionHandler` -> > `AlwaysContinueProductionExceptionHandler` > > Updated the typo error in KIP. > > - `DefaultProductionExceptionHandler` is not mentioned > > The `handleSerializationException` method in the > `ProductionExceptionHandler` interface will have default implementation > that is set to FAIL by default. > This is done to avoid any changes in the user implementation. So, I didn't > mentioned the `DefaultProductionExceptionHandler` class. Updated the KIP. > > - Why do you distinguish between `ClassCastException` and "any other > unchecked exception? Both second case seems to include the first one? > > In SinkNode.java#93 > < > https://github.com/apache/kafka/blob/87cc31c4e7ea36e7e832a1d02d71480a91a75293/streams/src/main/java/org/apache/kafka/streams/processor/internals/SinkNode.java#L93 > > > on > hitting `ClassCastException`, we are halting the streams as it's a fatal > error. > To keep the original behavior, I've to distinguish the exceptions. > > > On Thu, Dec 6, 2018 at 10:44 PM Matthias J. Sax <matth...@confluent.io> > wrote: > > > Well, that's exactly the point. The serializer should not be altered > > IMHO because this would have impact on other components. Also, for > > applications that use KafkaProducer directly, they can catch any > > serialization exception and react to it. Hence, I don't don't see a > > reason to change the serializer interface. > > > > Instead, it seems better to solve this issue in Streams by allowing to > > skip over a record for this case. > > > > Some more comments on the KIP: > > > > - the proposed method accepts a `ProducerRecord` -- it might be good to > > explain why this cannot be done in a type safe way (ie, missing generics) > > > > - `AlwaysProductionExceptionHandler` -> > > `AlwaysContinueProductionExceptionHandler` > > > > - `DefaultProductionExceptionHandler` is not mentioned > > > > - Why do you distinguish between `ClassCastException` and "any other > > unchecked exception? Both second case seems to include the first one? > > > > > > > > -Matthias > > > > On 12/6/18 8:35 AM, Matt Farmer wrote: > > > Ah, good point. > > > > > > Should we consider altering the serializer interface to permit not > > sending > > > the record? > > > > > > On Wed, Dec 5, 2018 at 9:23 PM Kamal Chandraprakash < > > > kamal.chandraprak...@gmail.com> wrote: > > > > > >> Matt, > > >> > > >> That's a good point. If these cases are handled in the serializer, > > then > > >> one cannot continue the stream processing by skipping the record. > > >> To continue, you may have to send a empty record serialized key/value > > (new > > >> byte[0]) to the downstream on hitting the error which may cause > > un-intended > > >> results. > > >> > > >> > > >> > > >> > > >> > > >> On Wed, Dec 5, 2018 at 8:41 PM Matt Farmer <m...@frmr.me> wrote: > > >> > > >>> Hi there, > > >>> > > >>> Thanks for this KIP. > > >>> > > >>> What’s the thinking behind doing this in ProductionExceptionHandler > > >> versus > > >>> handling these cases in your serializer implementation? > > >>> > > >>> On Mon, Dec 3, 2018 at 1:09 AM Kamal Chandraprakash < > > >>> kamal.chandraprak...@gmail.com> wrote: > > >>> > > >>>> Hello dev, > > >>>> > > >>>> I hope to initiate the discussion for KIP-399: Extend > > >>>> ProductionExceptionHandler to cover serialization exceptions. > > >>>> > > >>>> KIP: < > > >>>> > > >>>> > > >>> > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-399%3A+Extend+ProductionExceptionHandler+to+cover+serialization+exceptions > > >>>>> > > >>>> JIRA: https://issues.apache.org/jira/browse/KAFKA-7499 > > >>>> > > >>>> All feedbacks will be highly appreciated. > > >>>> > > >>>> Thanks, > > >>>> Kamal Chandraprakash > > >>>> > > >>> > > >> > > > > > > > >
