Yep, you can go ahead and call for a vote on the KIP On Thu, Sep 3, 2020 at 12:09 PM Gokul Srinivas <apa...@nym3r0s.cc> wrote:
> Sophie, > > That sounds fair. I've updated the KIP to state the same message for > backward compatibility to existing (albeit hacky) solutions. > > As this is my first ever contribution - is the next step to initiate the > voting on this KIP? > > -Gokul > > On 04-09-2020 00:34, Sophie Blee-Goldman wrote: > > I think the current proposal looks good to me. One minor suggestion I > have > > is to consider keeping the same error message: > > > > Failing batch since transaction was aborted > > > > > > When we were running into this issue in Streams and accidentally > rethrowing > > the KafkaException as fatal, we ended up checking the specific error > message > > of the KafkaException and swallowing the exception if it was equivalent > to > > the > > above. Obviously this was pretty hacky (hence the motivation for this > KIP) > > and > > luckily we found a way around this, but it makes me wonder if any > > applications > > out there might be doing the same. So maybe we should reuse the old error > > message just in case? > > > > Besides that, this KIP LGTM > > > > On Thu, Sep 3, 2020 at 5:23 AM Gokul Srinivas <apa...@nym3r0s.cc> wrote: > > > >> All, > >> > >> Gentle reminder - any comments on the line of thinking I mentioned in > >> the last email? I've updated the Exception to be named > >> "TransactionAbortedException" on the KIP confluence page. > >> > >> -Gokul > >> > >> On 01-09-2020 18:34, Gokul Srinivas wrote: > >>> Matthias, Sophie, Jason, > >>> > >>> Took another pass at understanding the internals and it seems to me > >>> like we should be extending the `ApiException` rather than the > >>> `RetriableException`. > >>> > >>> The check in question > >>> ===================== > >>> > >>> Do we abort any undrained batches that are present on this transaction > >>> if the transaction is in an aborting state? And, if we do, what would > >>> be the reason sent back to the user for aborting these batches? > >>> > >>> Logic for this > >>> ============== > >>> > >>> If the transaction `isAborting` and `hasAbortableError` and the > >>> `lastError()` is not empty -> then there has been some error which > >>> will cause / has caused the transaction to abort and this *is* a > >>> runtime exception. This same exception should be sent back to the user. > >>> > >>> If the transaction `isAborting` and `lastError()` is empty -> then for > >>> some unknown reason (maybe even user initiated, according to the > >>> tests), the transaction manager has started to abort the transaction. > >>> In this case, the newly proposed exception should be sent back to the > >>> user. > >>> > >>> Reasoning > >>> ========= > >>> > >>> Prima facie - I do not think this is a `RetriableException`. > >>> > >>> If the user has chosen to abort this transaction, then it would be up > >>> to the user to choose whether to retry the exception, in which case it > >>> is /*not*/ a `RetriableException`. > >>> > >>> If there is a case where the transaction manager has no error, but has > >>> started to abort the exception, we still do not retry the transaction, > >>> rather we abort any undrained batches - in which case, it is /*still > >>> not*/ a `RetriableException`. > >>> > >>> Does that sound right? > >>> > >>> -Gokul > >>> > >>> On 29-08-2020 01:17, Jason Gustafson wrote: > >>>> Hi Gokul, > >>>> > >>>> Thanks, I think it makes sense to use a separate exception type. +1 on > >>>> Sophie's suggestion for `TransactionAbortedException`. > >>>> > >>>> Extending from `RetriableException` seems reasonable as well. I guess > >>>> the > >>>> only question is whether it's safe to catch it as a > `RetriableException` > >>>> and apply common retry logic. For a transactional producer, my > >>>> expectation > >>>> is that the application would abort the transaction and retry it. > >>>> However, > >>>> if the transaction is already being aborted, maybe it would be better > to > >>>> skip the abort. It might be helpful to have an example which shows > >>>> how we > >>>> expect applications to handle this. > >>>> > >>>> Thanks, > >>>> Jason > >>>> > >>>> > >>>> > >>>> > >>>> > >>>> > >>>> On Thu, Aug 27, 2020 at 6:25 PM Sophie Blee-Goldman > >>>> <sop...@confluent.io> > >>>> wrote: > >>>> > >>>>> Hey Gokul, thanks for taking up this KIP! > >>>>> > >>>>> I agree with Matthias that directly extending KafkaException may not > be > >>>>> ideal, > >>>>> and we should instead extend APIException or RetriableException. Of > the > >>>>> two, > >>>>> I think APIException would be more appropriate. My understanding is > >>>>> that > >>>>> RetriableException is generally reserved for internally retriable > >>>>> exceptions > >>>>> whereas APIException is used for pseudo-fatal exceptions that > >>>>> require some > >>>>> user input as to how to proceed (eg ProducerFencedException) > >>>>> > >>>>> I also agree that the name could be a bit more concise. My personal > >>>>> vote > >>>>> would be for "TransactionAbortedException" which seems a bit more > >>>>> grammatically aligned with the other exceptions in Kafka. > >>>>> > >>>>> Cheers, > >>>>> Sophie > >>>>> > >>>>> On Thu, Aug 27, 2020 at 6:01 PM Matthias J. Sax <mj...@apache.org> > >>>>> wrote: > >>>>> > >>>>>> Thanks for the KIP. Looks good overall. > >>>>>> > >>>>>> However, I am wondering if the new exception should extend > >>>>>> `KafkaException`? It seems, extending `ApiException` or maybe even > >>>>>> `RetriableException` might be better? > >>>>>> > >>>>>> About the name itself. I would prefer something simpler like > >>>>>> `AbortedTransactionException`. > >>>>>> > >>>>>> Thoughts? > >>>>>> > >>>>>> > >>>>>> -Matthias > >>>>>> > >>>>>> > >>>>>> On 8/27/20 10:24 AM, Gokul Srinivas wrote: > >>>>>>> Hello all, > >>>>>>> > >>>>>>> I would like to propose the following KIP to throw a new non-fatal > >>>>>>> exception whilst aborting transactions with non-flushed data. This > >>>>>>> will > >>>>>>> help users distinguish non-fatal errors and potentially retry the > >>>>> batch. > >>>>>>> *Issue *- https://issues.apache.org/jira/browse/KAFKA-10186 > >>>>>>> <https://issues.apache.org/jira/browse/KAFKA-10186> > >>>>>>> > >>>>>>> *KIP *- > >>>>>>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-654:+Aborted+transaction+with+non-flushed+data+should+throw+a+non-fatal+exception > >>>>>>> < > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-654:+Aborted+transaction+with+non-flushed+data+should+throw+a+non-fatal+exception > >>>>>>> > >>>>>>> Please let me know how best we can proceed with this. > >>>>>>> > >>>>>>> -Gokul > >>>>>>> > >>>>>>> >
