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 <mj...@apache.org> 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. > > >
