>> To accept different types of records from multiple topologies, I have to >> define the ProducerRecord without generics.
Yes. It does make sense. My point was, that the KIP should mention/explain this explicitly to allow other not familiar with the code base to understand it more easily :) About `ClassCastException`: seems to be an implementation detail. No need to make it part of the KIP discussion. One more thing that came to my mind. We use the `RecordCollector` to write into all topics, ie, user output topics and internal repartition and changelog topics. For changelog topics, I think it does not make sense to allow skipping records if serialization fails? For internal repartitions topics, I am not sure if we should allow it or not. Would you agree with this? We should discuss the implication to derive a sound design. I was also just double checking the code, and it seems that the current `ProductionExceptionHandler` is applied for all topics. This seems to be incorrect to me. Seems we missed this case when doing KIP-210? (Or did we discuss this and I cannot remember? Might be worth to double check.) Last thought: of course, the handler will know which topic is affected and can provide a corresponding implementation. Was just wondering if we should be more strict? -Matthias On 12/6/18 10:01 AM, Kamal Chandraprakash 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 >>>>>> >>>>> >>>> >>> >> >> >
signature.asc
Description: OpenPGP digital signature
