Thanks Boyang for the KIP!
Like Matthias, I do also not know the producer internal well enough to
comment on the categorization. However, I think having a super exception
(e.g. RetriableException) that encodes if an exception is fatal or not
is cleaner, better understandable and less error-prone, because ideally
when you add a new non-fatal exception in future you just need to think
about letting it inherit from the super exception and all the rest of
the code will just behave correctly without the need to wrap the new
exception into another exception each time it is thrown (maybe it is
thrown at different location in the code).
As far as I understand the following statement from your previous e-mail
is the reason that currently such a super exception is not possible:
"Right now we have RetriableException type, if we are going to add a
`ProducerRetriableException` type, we have to put this new interface as
the parent of the RetriableException, because not all thrown non-fatal
exceptions are `retriable` in general for producer"
In the list of exceptions in your KIP, I found non-fatal exceptions that
do not inherit from RetriableException. I guess those are the ones you
are referring to in your statement:
InvalidProducerEpochException
InvalidPidMappingException
TransactionAbortedException
All of those exceptions are non-fatal and do not inherit from
RetriableException. Is there a reason for that? If they depended from
RetriableException we would be a bit closer to a super exception I
mention above.
With OutOfOrderSequenceException and UnknownProducerIdException, I think
to understand that their fatality depends on the type (i.e.
configuration) of the producer. That makes it difficult to have a super
exception that encodes the retriability as mentioned above. Would it be
possible to introduce exceptions that inherit from RetriableException
and that are thrown when those exceptions are caught from the brokers
and the type of the producer is such that the exceptions are retriable?
Best,
Bruno
On 04.12.20 19:34, Guozhang Wang wrote:
Thanks Boyang for the proposal! I made a pass over the list and here are
some thoughts:
0) Although this is not part of the public API, I think we should make sure
that the suggested pattern (i.e. user can always call abortTxn() when
handling non-fatal errors) are indeed supported. E.g. if the txn is already
aborted by the broker side, then users can still call abortTxn which would
not throw another exception but just be treated as a no-op.
1) *ConcurrentTransactionsException*: I think this error can also be
returned but not documented yet. This should be a non-fatal error.
2) *InvalidTxnStateException*: this error is returned from broker when txn
state transition failed (e.g. it is trying to transit to complete-commit
while the current state is not prepare-commit). This error could indicates
a bug on the client internal code or the broker code, OR a user error --- a
similar error is ConcurrentTransactionsException, i.e. if Kafka is bug-free
these exceptions would only be returned if users try to do something wrong,
e.g. calling abortTxn right after a commitTxn, etc. So I'm thinking it
should be a non-fatal error instead of a fatal error, wdyt?
3) *KafkaException*: case i "indicates fatal transactional sequence
(Fatal)", this is a bit conflicting with the *OutOfSequenceException* that
is treated as non-fatal. I guess your proposal is that
OutOfOrderSequenceException would be treated either as fatal with
transactional producer, or non-fatal with idempotent producer, is that
right? If the producer is only configured with idempotency but not
transaction, then throwing a TransactionStateCorruptedException for
non-fatal errors would be confusing since users are not using transactions
at all.. So I suggest we always throw OutOfSequenceException as-is (i.e.
not wrapped) no matter how the producer is configured, and let the caller
decide how to handle it based on whether it is only idempotent or
transactional itself.
4) Besides all the txn APIs, the `send()` callback / future can also throw
txn-related exceptions, I think this KIP should also cover this API as well?
5) This is related to 1/2) above: sometimes those non-fatal errors like
ConcurrentTxn or InvalidTxnState are not due to the state being corrupted
at the broker side, but maybe users are doing something wrong. So I'm
wondering if we should further distinguish those non-fatal errors between
a) those that are caused by Kafka itself, e.g. a broker timed out and
aborted a txn and later an endTxn request is received, and b) the user's
API call pattern is incorrect, causing the request to be rejected with an
error code from the broker. *TransactionStateCorruptedException* feels to
me more like for case a), but not case b).
Guozhang
On Wed, Dec 2, 2020 at 4:50 PM Boyang Chen <reluctanthero...@gmail.com>
wrote:
Thanks Matthias, I think your proposal makes sense as well, on the pro side
we could have a universally agreed exception type to be caught by the user,
without having an extra layer on top of the actual exceptions. I could see
some issue on downsides:
1. The exception hierarchy will be more complex. Right now we have
RetriableException type, if we are going to add a
`ProducerRetriableException` type, we have to put this new interface as the
parent of the RetriableException, because not all thrown non-fatal
exceptions are `retriable` in general for producer, for example
<
https://github.com/apache/kafka/blob/e275742f850af4a1b79b0d1bd1ac9a1d2e89c64e/clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java#L745
.
We could have an empty interface `ProducerRetriableException` to let all
the thrown exceptions implement for sure, but it's a bit unorthodox in the
way we deal with exceptions in general.
2. There are cases where we throw a KafkaException wrapping another
KafkaException as either fatal or non-fatal. If we use an interface to
solve #1, it is also required to implement another bloated exception class
which could replace KafkaException type, as we couldn't mark KafkaException
as retriable for sure.
3. In terms of the encapsulation, wrapping means we could limit the scope
of affection to the producer only, which is important since we don't want
shared exception types to implement a producer-related interface, such
as UnknownTopicOrPartitionException.
Best,
Boyang
On Wed, Dec 2, 2020 at 3:38 PM Matthias J. Sax <mj...@apache.org> wrote:
Thanks for the KIP Boyang!
Overall, categorizing exceptions makes a lot of sense. As I don't know
the producer internals well enough, I cannot comment on the
categorization in detail though.
What I am wondering is, if we should introduce an exception interface
that non-fatal exception would implement instead of creating a new class
that will wrap non-fatal exceptions? What would be the pros/cons for
both designs?
-Matthias
On 12/2/20 11:35 AM, Boyang Chen wrote:
Hey there,
I would like to start a discussion thread for KIP-691:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-691%3A+Enhance+Transactional+Producer+Exception+Handling
The KIP is aiming to simplify the exception handling logic for
transactional Producer users by classifying fatal and non-fatal
exceptions
and throw them correspondingly for easier catch and retry. Let me know
what
you think.
Best,
Boyang
