Re: [DISCUSS] KIP-1050: Consistent error handling for Transactions

[Lianet M.]

Hello, thanks for the KIP! After going through the KIP and discussion here
are some initial comments.

107 - I understand we’re  proposing a new
ProducerRetriableTransactionException, and changing existing exceptions to
inherit from it (the ones on the table below it).  The existing exceptions
inherit from RetriableException today, but with this KIP, they would
inherit from ProducerRetriableTransactionException which is not a
RetriableException ("*ProducerRetriableTransactionException extends
ApiException"*). Is my understanding correct? Wouldn’t this break
applications that could be handling/expecting RetriableExceptions today?
(Ie. apps dealing with TimeoutException on send , if they have
catch(RetriableException) or checks in the form of instanceOf
RetriableException, would need to change to the new
 ProducerRetriableTransactionException or the specific TimeoutException,
right?). I get this wouldn’t bring a problem for most of the retriable
exceptions on the table given that they end up being handled/retried
internally, but TimeoutException is tricky.


108 -  Regarding how we limit the scope of the change to the
producer/transactional API. TimeoutException is not only used in the
transactional API, but also in the consumer API, propagated to the user in
multiple api calls. Not clear to me how with this proposal we wouldn’t end
up with a consumer throwing a TimeoutException instanceOf
ProducerRetriableTransactionException? (Instead of instanceOf
RetriableException like it is today)? Again, potentially breaking apps but
also with a conceptually wrong consumer error?


109 - Similar to above, for exceptions like
UnknownTopicOrPartitionException, which are today instanceOf
RetriableException, if we’re saying they will be subclass of
ProducerRefreshRetriableTransactionException (ApiException) that will
affect the consumer logic too, where we do handle RetriableExceptions like
the unknownTopic, expecting RetriableException. This is all internal logic
and could be updated as needed of course, but without leaking
producer-specific groupings into the consumer I would expect.


110 - The KIP refers to the existing TransactionAbortableException (from
KIP-890), but on the public changes it refers to class
AbortableTransactionException extends ApiException. So are we proposing a
new exception type for this or reusing the existing one?

111 - I notice the proposed new exceptions, even though they seem like
abstract groupings, are not defined as "abstract". Is it intentional to
allow creation of instances of those?

Best!

Lianet


On Thu, Sep 12, 2024 at 6:26 AM Kaushik Raina 
wrote:

> Thanks for review Matthias
>
> 100/101 - Updated in KIP
>
> 104 - Added explicitly
> `For Producer-Retriable errors, the producer handles retries internally,
> keeping the failure details hidden from the application. Conversely, other
> types of exceptions will be surfaced to the application code for handling.`
>
> 105 - Grouped default exceptions explicitly
> `We will handle all default exceptions as generic unknown errors, which
> will be application recoverable. Below are few such exceptions:`
>
>
> On Sat, Aug 31, 2024 at 4:27 AM Matthias J. Sax  wrote:
>
> > Thanks for updating the KIP. It's much clearer now what you propose. I
> > have a bunch of question about the proposal:
> >
> >
> >
> > (100) nit (typo / missing word?):
> >
> > > We would new error types
> >
> >
> >
> > (101) `TransactionAbortableException`, `ProducerFencedException`, and
> > `UnknownProducerIdException` are listed twice in the tables.
> >
> >
> >
> > (102) You introduce a new exception `AbortableTransactionException`
> > which will only be extended by `TransactionAbortableException`. Given
> > that the existing TransactionAbortableException is not thrown by the
> > producer right now (no passed into the `Callback`), it seem if the
> > producer starts to throw/return the exiting
> > `TransactionAbortableException` or the new
> > `AbortableTransactionException` is would be an incompatible change?
> >
> >
> >
> > (103) It's unclear which method would throw the new
> > `AbortableTransactionException` and/or if this new exception might be
> > passe into the producer's send `Callback`.
> >
> >
> >
> > Btw: KIP-890 does not mention `TransactionAbortableException`... Does
> > KIP-890 need an update? KIP-890 only mentions a new error code
> > TRANSACTION_ABORTABLE -- or is this an implicit introduction of
> > TransactionAbortableException -- I am not familiar with the details how
> > core KIPs are written?
> >
> >
> >
> > (104) The KIP does not explicitly say, which of the new exceptions are
> > actually user facing? It seems only AbortableTransactionException,
> > ApplicationRecoverableTransactionException, and
> > InvalidConfiguationTransactionException are exception which user will be
> > able to catch (or handle vie the `Callback`), while
> > ProducerRetriableTransactionException and
> > ProducerRefreshRetriableTransactionException won't be thrown/return by
> >

Re: [DISCUSS] KIP-1050: Consistent error handling for Transactions

[Kaushik Raina]

Thanks for review Matthias

100/101 - Updated in KIP

104 - Added explicitly
`For Producer-Retriable errors, the producer handles retries internally,
keeping the failure details hidden from the application. Conversely, other
types of exceptions will be surfaced to the application code for handling.`

105 - Grouped default exceptions explicitly
`We will handle all default exceptions as generic unknown errors, which
will be application recoverable. Below are few such exceptions:`


On Sat, Aug 31, 2024 at 4:27 AM Matthias J. Sax  wrote:

> Thanks for updating the KIP. It's much clearer now what you propose. I
> have a bunch of question about the proposal:
>
>
>
> (100) nit (typo / missing word?):
>
> > We would new error types
>
>
>
> (101) `TransactionAbortableException`, `ProducerFencedException`, and
> `UnknownProducerIdException` are listed twice in the tables.
>
>
>
> (102) You introduce a new exception `AbortableTransactionException`
> which will only be extended by `TransactionAbortableException`. Given
> that the existing TransactionAbortableException is not thrown by the
> producer right now (no passed into the `Callback`), it seem if the
> producer starts to throw/return the exiting
> `TransactionAbortableException` or the new
> `AbortableTransactionException` is would be an incompatible change?
>
>
>
> (103) It's unclear which method would throw the new
> `AbortableTransactionException` and/or if this new exception might be
> passe into the producer's send `Callback`.
>
>
>
> Btw: KIP-890 does not mention `TransactionAbortableException`... Does
> KIP-890 need an update? KIP-890 only mentions a new error code
> TRANSACTION_ABORTABLE -- or is this an implicit introduction of
> TransactionAbortableException -- I am not familiar with the details how
> core KIPs are written?
>
>
>
> (104) The KIP does not explicitly say, which of the new exceptions are
> actually user facing? It seems only AbortableTransactionException,
> ApplicationRecoverableTransactionException, and
> InvalidConfiguationTransactionException are exception which user will be
> able to catch (or handle vie the `Callback`), while
> ProducerRetriableTransactionException and
> ProducerRefreshRetriableTransactionException won't be thrown/return by
> the producer into the app code?
>
>
>
> (105) `IllegalStateException` and `RuntimeException` which are Java
> exceptions are listed in the table of
> `ApplicationRecoverableTransactionException`: I think it is not valid to
> list them, as we cannot change their super-class.
>
>
>
> (106) `UnknownMemberIdException`, `IllegalGenerationException`, and
> `CorrelationIdMismatchException` are listed in the table of
> `ApplicationRecoverableTransactionException` but it seems they are not
> thrown/returned by the producer atm. If we start to throw/return either
> of them it seem to be a backward incompatible change.
>
>
>
> (106) Similarly to 105, `InvalidRecordException` and
> `InvalidRequiredAcksException` are listed in the table of
> `InvalidConfiguationTransactionException` but they seem not to be user
> facing.
>
>
>
>
> -Matthias
>
>
> On 7/25/24 8:50 AM, Kaushik Raina wrote:
> > Additionally,
> > - We will be depreciating KIP-691 in favour of KIP-1050.
> >
> >
> > On Fri, Jun 21, 2024 at 12:20 PM Kaushik Raina 
> wrote:
> >
> >> Thanks Matthias for feedback
> >> - We have updates KIP and grouped exceptions
> >>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1050%3A+Consistent+error+handling+for+Transactions#KIP1050:ConsistenterrorhandlingforTransactions-ExceptionTable
> >>
> >> - Regarding compatibility,  all changes in KIP are expected to be
> *backword
> >> compatible*.  We have updated KIP to make it clear.
> >>
> >>
> >> On Tue, Jun 11, 2024 at 4:50 AM Matthias J. Sax 
> wrote:
> >>
> >>> Thanks for this KIP. Great to see it. I would assume it will make
> >>> KIP-691 unnecessary?
> >>>
> >>> I don't think I fully understand the proposal yet. It's clear, that you
> >>> propose to add new sub-classed to group existing exceptions. But it's
> >>> not clear to me, which of the existing exceptions (which implement
> >>> ApiException directly right now) will get a new parent class and go
> into
> >>> the same group. You only list `InvalidProducerEpochException` which
> gets
> >>> `AbortableTransactionException` as new parent. It would help a lot, if
> >>> you could list out explicitly, which existing exceptions are grouped
> >>> together via which sub-class.
> >>>
> >>> It should be sufficient to just add a list for each group. For the
> newly
> >>> added exception classes, I would also omit all constructors etc and
> just
> >>> add a comment about it -- having constructors listed out does not add
> >>> much value to the KIP itself but makes it harder to read (it's
> >>> effectively noise we can avoid IMHO).
> >>>
> >>>
> >>>
> >>> I am also wondering about compatibility? If I read the section
> >>> correctly, you actually propose to introduce a non-backward-compatible
> >>> change?
> >>>

Re: [DISCUSS] KIP-1050: Consistent error handling for Transactions

[Justine Olshan]

Hey Matthias,

102/103) Sorry for the confusion on the TransactionAbortableException.
Here is the error -- we have an error name and an exception name -- this is
the same error mentioned in KIP-890. It is also implemented so that new
clients throw this error when transaction verification fails.
https://github.com/apache/kafka/blob/70dd577286de31ef20dc4f198e95f9b9e4479b47/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java#L405


I think the name can be standardized so we don't have two?

106) The Producer does return those errors when trying to commit
transactional offsets. See
https://github.com/apache/kafka/blob/70dd577286de31ef20dc4f198e95f9b9e4479b47/clients/src/main/java/org/apache/kafka/common/requests/TxnOffsetCommitResponse.java#L35

I will let Kaushik respond to some of the other points.

Justine

On Fri, Aug 30, 2024 at 3:57 PM Matthias J. Sax  wrote:

> Thanks for updating the KIP. It's much clearer now what you propose. I
> have a bunch of question about the proposal:
>
>
>
> (100) nit (typo / missing word?):
>
> > We would new error types
>
>
>
> (101) `TransactionAbortableException`, `ProducerFencedException`, and
> `UnknownProducerIdException` are listed twice in the tables.
>
>
>
> (102) You introduce a new exception `AbortableTransactionException`
> which will only be extended by `TransactionAbortableException`. Given
> that the existing TransactionAbortableException is not thrown by the
> producer right now (no passed into the `Callback`), it seem if the
> producer starts to throw/return the exiting
> `TransactionAbortableException` or the new
> `AbortableTransactionException` is would be an incompatible change?
>
>
>
> (103) It's unclear which method would throw the new
> `AbortableTransactionException` and/or if this new exception might be
> passe into the producer's send `Callback`.
>
>
>
> Btw: KIP-890 does not mention `TransactionAbortableException`... Does
> KIP-890 need an update? KIP-890 only mentions a new error code
> TRANSACTION_ABORTABLE -- or is this an implicit introduction of
> TransactionAbortableException -- I am not familiar with the details how
> core KIPs are written?
>
>
>
> (104) The KIP does not explicitly say, which of the new exceptions are
> actually user facing? It seems only AbortableTransactionException,
> ApplicationRecoverableTransactionException, and
> InvalidConfiguationTransactionException are exception which user will be
> able to catch (or handle vie the `Callback`), while
> ProducerRetriableTransactionException and
> ProducerRefreshRetriableTransactionException won't be thrown/return by
> the producer into the app code?
>
>
>
> (105) `IllegalStateException` and `RuntimeException` which are Java
> exceptions are listed in the table of
> `ApplicationRecoverableTransactionException`: I think it is not valid to
> list them, as we cannot change their super-class.
>
>
>
> (106) `UnknownMemberIdException`, `IllegalGenerationException`, and
> `CorrelationIdMismatchException` are listed in the table of
> `ApplicationRecoverableTransactionException` but it seems they are not
> thrown/returned by the producer atm. If we start to throw/return either
> of them it seem to be a backward incompatible change.
>
>
>
> (106) Similarly to 105, `InvalidRecordException` and
> `InvalidRequiredAcksException` are listed in the table of
> `InvalidConfiguationTransactionException` but they seem not to be user
> facing.
>
>
>
>
> -Matthias
>
>
> On 7/25/24 8:50 AM, Kaushik Raina wrote:
> > Additionally,
> > - We will be depreciating KIP-691 in favour of KIP-1050.
> >
> >
> > On Fri, Jun 21, 2024 at 12:20 PM Kaushik Raina 
> wrote:
> >
> >> Thanks Matthias for feedback
> >> - We have updates KIP and grouped exceptions
> >>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1050%3A+Consistent+error+handling+for+Transactions#KIP1050:ConsistenterrorhandlingforTransactions-ExceptionTable
> >>
> >> - Regarding compatibility,  all changes in KIP are expected to be
> *backword
> >> compatible*.  We have updated KIP to make it clear.
> >>
> >>
> >> On Tue, Jun 11, 2024 at 4:50 AM Matthias J. Sax 
> wrote:
> >>
> >>> Thanks for this KIP. Great to see it. I would assume it will make
> >>> KIP-691 unnecessary?
> >>>
> >>> I don't think I fully understand the proposal yet. It's clear, that you
> >>> propose to add new sub-classed to group existing exceptions. But it's
> >>> not clear to me, which of the existing exceptions (which implement
> >>> ApiException directly right now) will get a new parent class and go
> into
> >>> the same group. You only list `InvalidProducerEpochException` which
> gets
> >>> `AbortableTransactionException` as new parent. It would help a lot, if
> >>> you could list out explicitly, which existing exceptions are grouped
> >>> together via which sub-class.
> >>>
> >>> It should be sufficient to just add a list for each group. For the
> newly
> >>> added exception classes, I would also omit all constructors etc and
> just
> >>> add a comment about

Re: [DISCUSS] KIP-1050: Consistent error handling for Transactions

[Matthias J. Sax]

Thanks for updating the KIP. It's much clearer now what you propose. I 
have a bunch of question about the proposal:




(100) nit (typo / missing word?):

We would new error types 




(101) `TransactionAbortableException`, `ProducerFencedException`, and 
`UnknownProducerIdException` are listed twice in the tables.




(102) You introduce a new exception `AbortableTransactionException` 
which will only be extended by `TransactionAbortableException`. Given 
that the existing TransactionAbortableException is not thrown by the 
producer right now (no passed into the `Callback`), it seem if the 
producer starts to throw/return the exiting 
`TransactionAbortableException` or the new 
`AbortableTransactionException` is would be an incompatible change?




(103) It's unclear which method would throw the new 
`AbortableTransactionException` and/or if this new exception might be 
passe into the producer's send `Callback`.




Btw: KIP-890 does not mention `TransactionAbortableException`... Does 
KIP-890 need an update? KIP-890 only mentions a new error code 
TRANSACTION_ABORTABLE -- or is this an implicit introduction of 
TransactionAbortableException -- I am not familiar with the details how 
core KIPs are written?




(104) The KIP does not explicitly say, which of the new exceptions are 
actually user facing? It seems only AbortableTransactionException, 
ApplicationRecoverableTransactionException, and 
InvalidConfiguationTransactionException are exception which user will be 
able to catch (or handle vie the `Callback`), while 
ProducerRetriableTransactionException and 
ProducerRefreshRetriableTransactionException won't be thrown/return by 
the producer into the app code?




(105) `IllegalStateException` and `RuntimeException` which are Java 
exceptions are listed in the table of 
`ApplicationRecoverableTransactionException`: I think it is not valid to 
list them, as we cannot change their super-class.




(106) `UnknownMemberIdException`, `IllegalGenerationException`, and 
`CorrelationIdMismatchException` are listed in the table of 
`ApplicationRecoverableTransactionException` but it seems they are not 
thrown/returned by the producer atm. If we start to throw/return either 
of them it seem to be a backward incompatible change.




(106) Similarly to 105, `InvalidRecordException` and 
`InvalidRequiredAcksException` are listed in the table of 
`InvalidConfiguationTransactionException` but they seem not to be user 
facing.





-Matthias


On 7/25/24 8:50 AM, Kaushik Raina wrote:

Additionally,
- We will be depreciating KIP-691 in favour of KIP-1050.


On Fri, Jun 21, 2024 at 12:20 PM Kaushik Raina  wrote:


Thanks Matthias for feedback
- We have updates KIP and grouped exceptions
https://cwiki.apache.org/confluence/display/KAFKA/KIP-1050%3A+Consistent+error+handling+for+Transactions#KIP1050:ConsistenterrorhandlingforTransactions-ExceptionTable

- Regarding compatibility,  all changes in KIP are expected to be *backword
compatible*.  We have updated KIP to make it clear.


On Tue, Jun 11, 2024 at 4:50 AM Matthias J. Sax  wrote:


Thanks for this KIP. Great to see it. I would assume it will make
KIP-691 unnecessary?

I don't think I fully understand the proposal yet. It's clear, that you
propose to add new sub-classed to group existing exceptions. But it's
not clear to me, which of the existing exceptions (which implement
ApiException directly right now) will get a new parent class and go into
the same group. You only list `InvalidProducerEpochException` which gets
`AbortableTransactionException` as new parent. It would help a lot, if
you could list out explicitly, which existing exceptions are grouped
together via which sub-class.

It should be sufficient to just add a list for each group. For the newly
added exception classes, I would also omit all constructors etc and just
add a comment about it -- having constructors listed out does not add
much value to the KIP itself but makes it harder to read (it's
effectively noise we can avoid IMHO).



I am also wondering about compatibility? If I read the section
correctly, you actually propose to introduce a non-backward-compatible
change?


Based on type of exception thrown, user needs to change their exception

catching logic to take actions against their exception handling.

Ie, an application cannot be upgrade w/o code changes? I am not sure if
this is acceptable?

I think it would be much better (not sure if feasible) to keep the old
behavior and let users opt-in / enable the new semantics via a config.
If the new behavior is disabled, we could log a WARN that the app should
upgrade to work with the new semantics, and we would only enforce the
new behavior in a later major release.

Thoughts?



-Matthias






On 6/7/24 4:06 AM, Kaushik Raina wrote:

Thank you Andrew for feedback

1. We are suggesting to only update subclasses of
o.a.k.common.errors.ApiException, which are used in transactions. All

such

subclasses are mentioned in Exception table
<

https://c

Re: [DISCUSS] KIP-1050: Consistent error handling for Transactions

[Kaushik Raina]

Additionally,
- We will be depreciating KIP-691 in favour of KIP-1050.


On Fri, Jun 21, 2024 at 12:20 PM Kaushik Raina  wrote:

> Thanks Matthias for feedback
> - We have updates KIP and grouped exceptions
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1050%3A+Consistent+error+handling+for+Transactions#KIP1050:ConsistenterrorhandlingforTransactions-ExceptionTable
>
> - Regarding compatibility,  all changes in KIP are expected to be *backword
> compatible*.  We have updated KIP to make it clear.
>
>
> On Tue, Jun 11, 2024 at 4:50 AM Matthias J. Sax  wrote:
>
>> Thanks for this KIP. Great to see it. I would assume it will make
>> KIP-691 unnecessary?
>>
>> I don't think I fully understand the proposal yet. It's clear, that you
>> propose to add new sub-classed to group existing exceptions. But it's
>> not clear to me, which of the existing exceptions (which implement
>> ApiException directly right now) will get a new parent class and go into
>> the same group. You only list `InvalidProducerEpochException` which gets
>> `AbortableTransactionException` as new parent. It would help a lot, if
>> you could list out explicitly, which existing exceptions are grouped
>> together via which sub-class.
>>
>> It should be sufficient to just add a list for each group. For the newly
>> added exception classes, I would also omit all constructors etc and just
>> add a comment about it -- having constructors listed out does not add
>> much value to the KIP itself but makes it harder to read (it's
>> effectively noise we can avoid IMHO).
>>
>>
>>
>> I am also wondering about compatibility? If I read the section
>> correctly, you actually propose to introduce a non-backward-compatible
>> change?
>>
>> > Based on type of exception thrown, user needs to change their exception
>> catching logic to take actions against their exception handling.
>>
>> Ie, an application cannot be upgrade w/o code changes? I am not sure if
>> this is acceptable?
>>
>> I think it would be much better (not sure if feasible) to keep the old
>> behavior and let users opt-in / enable the new semantics via a config.
>> If the new behavior is disabled, we could log a WARN that the app should
>> upgrade to work with the new semantics, and we would only enforce the
>> new behavior in a later major release.
>>
>> Thoughts?
>>
>>
>>
>> -Matthias
>>
>>
>>
>>
>>
>>
>> On 6/7/24 4:06 AM, Kaushik Raina wrote:
>> > Thank you Andrew for feedback
>> >
>> > 1. We are suggesting to only update subclasses of
>> > o.a.k.common.errors.ApiException, which are used in transactions. All
>> such
>> > subclasses are mentioned in Exception table
>> > <
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1050%3A+Consistent+error+handling+for+Transactions#KIP1050:ConsistenterrorhandlingforTransactions-ExceptionTable
>> >
>> >
>> > 2. "Producer-Recoverable" corresponds to the AbortableException. I have
>> > updated comments on each exception type.
>> >
>> > 3. Yes, it's correct that by adding a "Retriable" exception, it
>> simplifies
>> > the determination of which errors can be retried internally. In the
>> Exception
>> > table
>> > <
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1050%3A+Consistent+error+handling+for+Transactions#KIP1050:ConsistenterrorhandlingforTransactions-ExceptionTable
>> >
>> > mentioned
>> > in the "Proposed Changes" section, the "Expected Handling" column
>> signifies
>> > the handling for each error type. Please let me know if any further
>> > clarification is needed.
>> >
>> > 4a. Yes, that is correct. For clarity, only one constructor has been
>> > mentioned in the KIP. An ellipsis has been added as a placeholder,
>> > indicating that there are additional functions in the class but they are
>> > not explicitly specified.
>> > 4b. Updated in the KIP.
>> >
>> > 5. TopicAuthorizationException extends "Invalid Configuration". "Invalid
>> > Configuration" type can be resolved either by dynamically updating the
>> > configuration, which does not require a restart, or by statically
>> updating
>> > it by restarting the application. It is at the application's discretion
>> how
>> > they want to handle each "Invalid Configuration" type.
>> >
>> > I have added Client side handling example
>> > <
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1050%3A+Consistent+error+handling+for+Transactions#KIP1050:ConsistenterrorhandlingforTransactions-Clientsidecodeexample
>> >
>> > in
>> > KIP. Hope that helps.
>> >
>>
>

Re: [DISCUSS] KIP-1050: Consistent error handling for Transactions

[Kaushik Raina]

Thanks Matthias for feedback
- We have updates KIP and grouped exceptions
https://cwiki.apache.org/confluence/display/KAFKA/KIP-1050%3A+Consistent+error+handling+for+Transactions#KIP1050:ConsistenterrorhandlingforTransactions-ExceptionTable

- Regarding compatibility,  all changes in KIP are expected to be *backword
compatible*.  We have updated KIP to make it clear.


On Tue, Jun 11, 2024 at 4:50 AM Matthias J. Sax  wrote:

> Thanks for this KIP. Great to see it. I would assume it will make
> KIP-691 unnecessary?
>
> I don't think I fully understand the proposal yet. It's clear, that you
> propose to add new sub-classed to group existing exceptions. But it's
> not clear to me, which of the existing exceptions (which implement
> ApiException directly right now) will get a new parent class and go into
> the same group. You only list `InvalidProducerEpochException` which gets
> `AbortableTransactionException` as new parent. It would help a lot, if
> you could list out explicitly, which existing exceptions are grouped
> together via which sub-class.
>
> It should be sufficient to just add a list for each group. For the newly
> added exception classes, I would also omit all constructors etc and just
> add a comment about it -- having constructors listed out does not add
> much value to the KIP itself but makes it harder to read (it's
> effectively noise we can avoid IMHO).
>
>
>
> I am also wondering about compatibility? If I read the section
> correctly, you actually propose to introduce a non-backward-compatible
> change?
>
> > Based on type of exception thrown, user needs to change their exception
> catching logic to take actions against their exception handling.
>
> Ie, an application cannot be upgrade w/o code changes? I am not sure if
> this is acceptable?
>
> I think it would be much better (not sure if feasible) to keep the old
> behavior and let users opt-in / enable the new semantics via a config.
> If the new behavior is disabled, we could log a WARN that the app should
> upgrade to work with the new semantics, and we would only enforce the
> new behavior in a later major release.
>
> Thoughts?
>
>
>
> -Matthias
>
>
>
>
>
>
> On 6/7/24 4:06 AM, Kaushik Raina wrote:
> > Thank you Andrew for feedback
> >
> > 1. We are suggesting to only update subclasses of
> > o.a.k.common.errors.ApiException, which are used in transactions. All
> such
> > subclasses are mentioned in Exception table
> > <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1050%3A+Consistent+error+handling+for+Transactions#KIP1050:ConsistenterrorhandlingforTransactions-ExceptionTable
> >
> >
> > 2. "Producer-Recoverable" corresponds to the AbortableException. I have
> > updated comments on each exception type.
> >
> > 3. Yes, it's correct that by adding a "Retriable" exception, it
> simplifies
> > the determination of which errors can be retried internally. In the
> Exception
> > table
> > <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1050%3A+Consistent+error+handling+for+Transactions#KIP1050:ConsistenterrorhandlingforTransactions-ExceptionTable
> >
> > mentioned
> > in the "Proposed Changes" section, the "Expected Handling" column
> signifies
> > the handling for each error type. Please let me know if any further
> > clarification is needed.
> >
> > 4a. Yes, that is correct. For clarity, only one constructor has been
> > mentioned in the KIP. An ellipsis has been added as a placeholder,
> > indicating that there are additional functions in the class but they are
> > not explicitly specified.
> > 4b. Updated in the KIP.
> >
> > 5. TopicAuthorizationException extends "Invalid Configuration". "Invalid
> > Configuration" type can be resolved either by dynamically updating the
> > configuration, which does not require a restart, or by statically
> updating
> > it by restarting the application. It is at the application's discretion
> how
> > they want to handle each "Invalid Configuration" type.
> >
> > I have added Client side handling example
> > <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1050%3A+Consistent+error+handling+for+Transactions#KIP1050:ConsistenterrorhandlingforTransactions-Clientsidecodeexample
> >
> > in
> > KIP. Hope that helps.
> >
>

Re: [DISCUSS] KIP-1050: Consistent error handling for Transactions

[Matthias J. Sax]

Thanks for this KIP. Great to see it. I would assume it will make 
KIP-691 unnecessary?


I don't think I fully understand the proposal yet. It's clear, that you 
propose to add new sub-classed to group existing exceptions. But it's 
not clear to me, which of the existing exceptions (which implement 
ApiException directly right now) will get a new parent class and go into 
the same group. You only list `InvalidProducerEpochException` which gets 
`AbortableTransactionException` as new parent. It would help a lot, if 
you could list out explicitly, which existing exceptions are grouped 
together via which sub-class.


It should be sufficient to just add a list for each group. For the newly 
added exception classes, I would also omit all constructors etc and just 
add a comment about it -- having constructors listed out does not add 
much value to the KIP itself but makes it harder to read (it's 
effectively noise we can avoid IMHO).




I am also wondering about compatibility? If I read the section 
correctly, you actually propose to introduce a non-backward-compatible 
change?



Based on type of exception thrown, user needs to change their exception 
catching logic to take actions against their exception handling.


Ie, an application cannot be upgrade w/o code changes? I am not sure if 
this is acceptable?


I think it would be much better (not sure if feasible) to keep the old 
behavior and let users opt-in / enable the new semantics via a config. 
If the new behavior is disabled, we could log a WARN that the app should 
upgrade to work with the new semantics, and we would only enforce the 
new behavior in a later major release.


Thoughts?



-Matthias






On 6/7/24 4:06 AM, Kaushik Raina wrote:

Thank you Andrew for feedback

1. We are suggesting to only update subclasses of
o.a.k.common.errors.ApiException, which are used in transactions. All such
subclasses are mentioned in Exception table


2. "Producer-Recoverable" corresponds to the AbortableException. I have
updated comments on each exception type.

3. Yes, it's correct that by adding a "Retriable" exception, it simplifies
the determination of which errors can be retried internally. In the Exception
table

mentioned
in the "Proposed Changes" section, the "Expected Handling" column signifies
the handling for each error type. Please let me know if any further
clarification is needed.

4a. Yes, that is correct. For clarity, only one constructor has been
mentioned in the KIP. An ellipsis has been added as a placeholder,
indicating that there are additional functions in the class but they are
not explicitly specified.
4b. Updated in the KIP.

5. TopicAuthorizationException extends "Invalid Configuration". "Invalid
Configuration" type can be resolved either by dynamically updating the
configuration, which does not require a restart, or by statically updating
it by restarting the application. It is at the application's discretion how
they want to handle each "Invalid Configuration" type.

I have added Client side handling example

in
KIP. Hope that helps.

Re: [DISCUSS] KIP-1050: Consistent error handling for Transactions

[Kaushik Raina]

Thank you Andrew for feedback

1. We are suggesting to only update subclasses of
o.a.k.common.errors.ApiException, which are used in transactions. All such
subclasses are mentioned in Exception table


2. "Producer-Recoverable" corresponds to the AbortableException. I have
updated comments on each exception type.

3. Yes, it's correct that by adding a "Retriable" exception, it simplifies
the determination of which errors can be retried internally. In the Exception
table

mentioned
in the "Proposed Changes" section, the "Expected Handling" column signifies
the handling for each error type. Please let me know if any further
clarification is needed.

4a. Yes, that is correct. For clarity, only one constructor has been
mentioned in the KIP. An ellipsis has been added as a placeholder,
indicating that there are additional functions in the class but they are
not explicitly specified.
4b. Updated in the KIP.

5. TopicAuthorizationException extends "Invalid Configuration". "Invalid
Configuration" type can be resolved either by dynamically updating the
configuration, which does not require a restart, or by statically updating
it by restarting the application. It is at the application's discretion how
they want to handle each "Invalid Configuration" type.

I have added Client side handling example

in
KIP. Hope that helps.

Re: [DISCUSS] KIP-1050: Consistent error handling for Transactions

[Andrew Schofield]

Hi Kaushik,
Thanks for the KIP. This is definitely an area that needs clearing up so it’s 
good to see it.

A few initial questions.

1. If I understand correctly, you are proposing to change the superclass of all
subclasses of o.a.k.common.errors.ApiException which can be thrown by the 
producer
or transaction APIs. Is this accurate?

2. You have 4 exception types (and 2 subtypes) in the list in Proposed Changes. 
So, I
would expect 5 new exception classes with names with a direct correspondence to
the list of types, or at least an explicit statement of which of the new 
exceptions
maps to each of the types. For example, “Producer-Recoverable” doesn’t seem to
have a new exception. I’m a bit confused.

3. Some of the error types, the “producer retriable” ones, can be handled
directly within the producer code and do not need to be surfaced to the 
applications.
I suppose this means there is no need to make any change at all for these 
exceptions.
By changing these exceptions too, I suppose it makes it simple in the producer 
to
figure out which errors can be retried internally.

Could you be explicit about which exceptions are going to be changed and which
class is the new superclass? For one thing, having a table would make it obvious
to the code reviewers whether the intended change was being made.

4. A few nits about the Java code snippet for the new exception types.

a. ApiException has 4 constructors: (), (String), (Throwable) and (String, 
Throwable).
I think you’ll need 4 constructors for each of your new exceptions.

b. In some cases, the class names and the constructor names are inconsistent.

5. I believe that a producer application which received 
TopicAuthorizationException
could continue to use the same Producer attempting to use the topic while the
administrator fixed the ACLs to grant access, and then the operations would 
start
working as soon as the access was in place. Is this true for the authorization
exceptions in this KIP? Does it vary by resource type, such as transactional ID?

Thanks,
Andrew

> On 6 Jun 2024, at 12:57, Kaushik Raina  wrote:
>
> Hi everyone, I would like to start a discussion thread for KIP-1050:
> Consistent error handling for Transactions
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1050%3A+Consistent+error+handling+for+Transactions
>
>
> Thanks
> Kaushik Raina

[DISCUSS] KIP-1050: Consistent error handling for Transactions

[Kaushik Raina]

Hi everyone, I would like to start a discussion thread for KIP-1050:
Consistent error handling for Transactions
https://cwiki.apache.org/confluence/display/KAFKA/KIP-1050%3A+Consistent+error+handling+for+Transactions


Thanks
Kaushik Raina