Subject: [DISCUSS] KIP-399: Extend ProductionExceptionHandler to cover serialization exceptions


>> 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 <[EMAIL PROTECTED]>
> 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 <
Comment: GPGTools - https://gpgtools.org

iQIzBAEBCgAdFiEESn/iOv2tmCkcP0KLVp2sL37kObwFAlwJbEYACgkQVp2sL37k
ObySbxAAiDPkgzlP/fHNr1CMUYVXpDIWymgpGXycTFUpkCDdkBEYXRmNdLL2++Xq
ReAd8htyWyxecxsHpmkmExc5rnCVKZ4XXoB4zfA09PqwZ9dlzBu/8382Rg6yh1Y1
BD9ztaQYko01bPawBoefAtOtDOIWbFy5d1R3EBQXyWKCh9w2qJpmuGrsEARTTcsa
8f2qM8bEQ9oaXuCXnMyrsk98f+if1/Ua4xpu1LG0+zsVHqvk4jckB3uIPzNuCKA9
ft70ACbI0Qodqp4h0tYcrvysvgKmdV01G/Dl+3ZmRzXDTMdHoAtJ/R9ywIkAh+W4
MsZVVrN28MarwZgDduzk/S67PE/DEoCDBKh0VHzyRp9kGGLq3PEL6FghpQqDzECV
KIBx8AreTjjEnUJ5lvtAI98mA71VCa+qbrnbJDoeofZtSymV5ZWDlhcOb2fu/4bM
pIvGb62PxxKT201+zaHILqkxE/j2Jfbx5DRwwJlRxPit5PQknb3tAvXuUq5vzFdd
166HF24YDgdziFMEO3BVKxkGZ3RgyvFayHJ3UiGtU+8oEUJWlgl73htARpOreOmH
1xqzrBvUN8BRU9tY267QZuJCheaFsEXbJsBAemtGzrfhldLB3IFv5LE+JHLMqER2
hIcQZGKISa7tcmrMp3/RW6IKOawlyihcMyPHRQShVrmqxNXYoSE=
=j6jI