Thanks Lianet for review LM1 & LM2: We will extend parent classes only to maintain the hierarchy
For TopicAuthorizationException and GroupAuthorizationException, we will extend parent class AuthorizationException. So new hierarchy will be "AuthorizationException < InvalidConfigurationException < ApiException" For UnknownTopicOrPartitionException and NotLeaderOrFollowerException, we will extend parent class InvalidMetadataException. So new hierarchy will be "InvalidMetadataException < RefreshRetriableException < RetriableException < ApiException" LM3: CommitFailedException will not be extended because its parent class KafkaException is expected to be treated as application recoverable as mentioned in KIP. I have added a `comment` section in the KIP-1050 table to include LM1 & LM2 & LM3 details. LM4: StaleMemberEpochException is thrown as IllegalGenerationException in Txn https://github.com/apache/kafka/blob/trunk/group-coordinator/src/main/java/org/apache/kafka/coordinator/group/OffsetMetadataManager.java#L367-L379 . So it need not be part of KIP On Thu, Nov 7, 2024 at 1:05 AM Lianet M. <liane...@gmail.com> wrote: > Hello, sorry for the late reply, took another pass after the latest > updates. Most of the proposed groupings and handling seem good and very > helpful indeed from the producer perspective. I just have some more > comments related to the changes in the exception hierarchy, that are not > scoped to the producer: > > > LM1 - Regarding TopicAuthorizationException and > GroupAuthorizationException. We’re proposing to change their parent class > to InvalidConfigurationException, which extends ApiException (vs their > current parent AuthorizationException). This would be a breaking change for > apps expecting/handling instanceOf AuthorizationException (and an > unexpected one I would say, given that TopicAuthorizationException and > GroupAuthorizationException are indeed “authorization errors”). > > > LM2 - Similarly, regarding UnknownTopicOrPartitionException and > NotLeaderOrFollowerException: we’re proposing to change parent class to > RefreshRetriableException (which extends Retriable), but that would mean > that these exceptions wouldn’t be instanceOf InvalidMetadataException > anymore (as they are today). This would be a breaking change for client > apps dealing with instanceOf InvalidMetadata right? > > > LM3 - Regarding CommitFailedException, I’m simply concerned that the move > may not be conceptually right? We’re proposing to change its parent class > to ApplicationRecoverableException, which would make CommitFailedException > land under the ApiException umbrella. ApiException is intended for errors > that are part of the protocol, and this CommitFailedException is not (it’s > just an exception generated on the client side based on different > protocol-level errors). Not that it will have an impact on existing apps > given that it would still be a KafkaException as it is today, but wanted to > point this out. > > > LM4 - Under ApplicationRecoverableException, I wonder if we should also > consider StaleMemberEpochException (just for consistency, alongside > FencedInstaceIdException, UnknownMemberIdException,...) > > > Thanks! > > > Lianet > > On Thu, Oct 24, 2024 at 6:25 AM Kaushik Raina <kra...@confluent.io.invalid > > > wrote: > > > Thanks Artem for valuable comments > > > > I have incorporated them into the updated KIP. > > > > > > On Sat, Oct 19, 2024 at 3:31 AM Artem Livshits > > <alivsh...@confluent.io.invalid> wrote: > > > > > Hi Kaushik, > > > > > > Thank you for the KIP! I think it'll make writing transactional > > > application easier and less error prone. I have a couple comments: > > > > > > AL1. The keep proposes changing the semantics > > > of UnknownProducerIdException. Currently, this error is never returned > > by > > > the broker so we cannot validate whether changing semantics is going to > > be > > > compatible with a future use (if we ever return it again) and the > future > > > broker wouldn't know which sematnics the client supports. I think for > > now > > > we could just add a comment in the code that this error is never > returned > > > by the broker and if we ever add it to the broker we'd need to make > sure > > > it's categorized properly and bump the API version so that the broker > > knows > > > if the client supports required semantics. > > > > > > AL2. The exception classification looks good to me from the producer > > > perspective: we have retriable, abortable, etc. categories. The > > retriable > > > errors can be retried within producer safely without losing > idempotence, > > > the messages would be retried with the same sequence numbers and we > won't > > > have duplicates. However, if a retriable error (e.g. TimeoutException) > > is > > > thrown to the application, it's not safe to retry the produce, because > > the > > > messages will get new sequence numbers and if the original message in > > fact > > > succeeded, the new messages would become duplicates, losing the exactly > > > once semantics. Thus, if a retriable exception is thrown from a > > > transactional "produce" we should abort the transaction to guarantee > that > > > we won't have duplicates when we produce messages again. To avoid > > > confusion, I propose to change the transactional "produce" to never > > return > > > a retriable error, i.e. any retriable error would be translated > > > into TransactionAbortableException (the error message could be copied > > from > > > the original error) before getting thrown from transactional "produce". > > > This would avoid bugs in the applicatoin -- even if the application > > > developer gets confused and decides to retry on TimeoutException, the > > buggy > > > code would never run. > > > > > > Another change that we should make is to make sure .abortTransaction() > > > should never throw producer-abortable exception. > > > > > > The handling of retriable errors for other APIs can stay unchanged -- > > > .commitTransaction() and .abortTransaction() are idempotent and can be > > > safely retried, read-only APIs can always be safely retried, and for > > > non-transactional producer we don't have a good "path back to normal" > > > anyway -- we cannot reliably abort messages with unknown fate. > > > > > > -Artem > > > > > > On Fri, Oct 4, 2024 at 5:11 AM Lianet M. <liane...@gmail.com> wrote: > > > > > > > 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 > > > <kra...@confluent.io.invalid > > > > > > > > > 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 <mj...@apache.org> > > > > 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 < > > > kra...@confluent.io> > > > > > > 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 < > > 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. > > > > > > >>>> > > > > > > >>> > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > >
