Thanks Lianet for valuable comments
107) & 108) We have updated group exception names, which will prevent
error in current applications which are handing producer/consumer exceptions
```
ProducerRetriableTransactionException -> RetriableException
ProducerRefreshRetriableTransactionException -> RefreshRetriableException
ApplicationRecoverableTransactionException ->
ApplicationRecoverableException
InvalidConfigurationTransactionException -> InvalidConfigurationException
```
109) We have updated KIP with
```
RefreshRetriableException extends RetriableException
```
This will make current applications which are handing RetriableException
compatible with RefreshRetriableException
110) Updated KIP, we will be using existing one
111) Updated KIP to use abstract type.
On Fri, Oct 4, 2024 at 5:41 PM 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.
> > > >>>>
> > > >>>
> > > >>
> > > >
> > >
> >
>
