Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi Mathias, > [AL1] While I see the point, I would think having a different callback for every exception might not really be elegant? I'm not sure how to assess the level of elegance of the proposal, but I can comment on the technical characteristics: 1. Having specific interfaces that codify

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

My concern with respect to it being fragile: the code that ensures the error type is internal to the producer. Someone may see it and say, I want to add such and such error. This looks like internal code, so I don't need a KIP, and then they can change it to whatever they want thinking it is

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi Alieh, Continuing prior discussions: 1) Regarding the "flexibility" discussion, my overarching point is that I don't see the point in allowing for this kind of pluggable logic without also covering more scenarios. Take example 2 in the KIP: if we're going to implement retries only on

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Very interesting discussion. Seems a central point is the question about "how generic" we approach this, and some people think we need to be conservative and others think we should try to be as generic as possible. Personally, I think following a limited scope for this KIP (by explicitly

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi Alieh, Thanks for the KIP. The motivation talks about very specific cases, but the interface is generic. [AL1] If the interface evolves in the future I think we could have the following confusion: 1. A user implemented SWALLOW action for both RecordTooLargeException and

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Alieh and Chris, Thanks for clarifying 1) but I saw the motivation. I guess I just didn't understand how that would be ensured on the producer side. I saw the sample code -- is it just an if statement checking for the error before the handler is invoked? That seems a bit fragile. Can you clarify

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi, Thank you, Chris and Justine, for the feedback. @Chris 1) Flexibility: it has two meanings. The first meaning is the one you mentioned. We are going to cover more exceptions in the future, but as Justine mentioned, we must be very conservative about adding more exceptions. Additionally,

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi Justine, The method signatures for the interface are indeed open-ended, but the KIP states that its uses will be limited. See the motivation section: > We believe that the user should be able to develop custom exception handlers for managing producer exceptions. On the other hand, this will

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi Alieh, I was out for KSB and then was also sick. :( To your point 1) Chris, I don't think it is limited to two specific scenarios, since the interface accepts a generic Exception e and can be implemented to check if that e is an instanceof any exception. I didn't see anywhere that specific

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi Alieh, Sorry for the delay, I've been out sick. I still have some thoughts that I'd like to see addressed before voting. 1) If flexibility is the motivation for a pluggable interface, why are we only limiting the uses for this interface to two very specific scenarios? Why not also allow,

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi all, A summary of the KIP and the discussions: The KIP introduces a handler interface for Producer in order to handle two exceptions: RecordTooLargeException and UnknownTopicOrPartitionException. The handler handles the exceptions per-record. - Do we need this handler? [Motivation and

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi Alieh, Thanks for the updates! Comments inline... > On Apr 25, 2024, at 1:10 PM, Alieh Saeedi > wrote: > > Hi all, > > Thanks a lot for the constructive feedbacks! > > > > Addressing some of the main concerns: > > > - The `RecordTooLargeException` can be thrown by broker, producer

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi all, Thanks a lot for the constructive feedbacks! Addressing some of the main concerns: - The `RecordTooLargeException` can be thrown by broker, producer and consumer. Of course, the `ProducerExceptionHandler` interface is introduced to affect only the exceptions thrown from the producer.

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Thanks Alieh for the updates. I'm a little concerned about the design pattern here. It seems like we want specific usages, but we are packaging it as a generic handler. I think we tried to narrow down on the specific errors we want to handle, but it feels a little clunky as we have a generic

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi Alieh, Thanks for the KIP! A few questions: K1. What is the expected behavior for the producer if it generates a RecordTooLargeException, but the handler returns RETRY? K2. How do we determine which Record was responsible for the UnknownTopicOrPartitionException since we get that response

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Alieh, Having run into issues with not being able to handle producer failures, I think this is good functionality to have. With this new functionality proposed at the Producer level, how would ecosystems that sit on top of it function? Specifically, Connect was updated a few years ago to allow

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Thanks Matthias. I changed it to `custom.exception.handler` Alieh On Tue, Apr 23, 2024 at 8:47 AM Matthias J. Sax wrote: > Thanks Alieh! > > A few nits: > > > 1) The new config we add for the producer should be mentioned in the > "Public Interfaces" section. > > 2) Why do we use `producer.`

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Thanks Alieh! A few nits: 1) The new config we add for the producer should be mentioned in the "Public Interfaces" section. 2) Why do we use `producer.` prefix for a *producer* config? Should it be `exception.handler` only? -Matthias On 4/22/24 7:38 AM, Alieh Saeedi wrote: Thank you

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Thank you all for the feedback! Addressing the main concern: The KIP is about giving the user the ability to handle producer exceptions, but to be more conservative and avoid future issues, we decided to be limited to a short list of exceptions. I included *RecordTooLargeExceptin* and

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Thanks for the KIP Alieh! It addresses an important case for error handling. I agree that using this handler would be an expert API, as mentioned by a few people. But I don't think it would be a reason to not add it. It's always a tricky tradeoff what to expose to users and to avoid foot guns,

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi Alieh, Thanks for the KIP. Exception handling in the Kafka producer and consumer is really not ideal. It’s even harder working out what’s going on with the consumer. I’m a bit nervous about this KIP and I agree with Chris that it could do with additional motivation. This would be an

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi Alieh, Thanks for the KIP! The issue with writing to non-existent topics is particularly frustrating for users of Kafka Connect and has been the source of a handful of Jira tickets over the years. My thoughts: 1. An additional detail we can add to the motivation (or possibly rejected

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hey Alieh, some comments: * "Compatibility" section wasn't clear to me. Are we just introducing the interfaces or are we changing the default behavior? If so, that should be explained in more detail. * If we are introducing a new interface `ClientExceptionHandler`, what is it going to be used

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hey Alieh, I echo what Omnia says, I'm not sure I understand the implications of the change and I think more detail is needed. This comment also confused me a bit: * {@code ClientExceptionHandler} that continues the transaction even if a record is too large. * Otherwise, it makes the transaction

Re: [DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi Alieh, Thanks for the KIP! I have couple of comments - You mentioned in the KIP motivation, > Another example for which a production exception handler could be useful is > if a user tries to write into a non-existing topic, which returns a retryable > error code; with infinite retries, the

[DISCUSS] KIP-1038: Add Custom Error Handler to Producer

Hi all, Here is the KIP-1038: Add Custom Error Handler to Producer. I look forward to your feedback! Cheers, Alieh