urbandan commented on code in PR #13591:
URL: https://github.com/apache/kafka/pull/13591#discussion_r1173405147
##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java:
##########
@@ -968,13 +968,31 @@ private void transitionTo(State target) {
}
private void transitionTo(State target, RuntimeException error) {
+ transitionTo(target, error, false);
Review Comment:
> > This is a call chain where we transition on direct user action,
shouldn't this be throwing? KafkaProducer.send -> KafkaProducer.doSend ->
maybeTransitionToErrorState -> transitionToAbortableError -> transitionTo
>
> Since maybeTransitionToErrorState is being called from inside a catch
block, if we did throw an exception, it would mask the root issue
(ApiException), right?
I think masking the ApiException is better than silently transitioning into
fatal state - if transitionToAbortableError tries going into abortable state,
that ApiException is probably something that the calling code can handle, and
try to recover by aborting. If we still throw that exception, but in reality
the internal state is fatal already, that is a violation of the API, isn't it?
> After the abortableError call, if the state transition was invalid, then
currentState would be FATAL_ERROR. That has the same effect as the last two
branches of the if statement that call the fatalError method, right?
>
> Would simply skipping the reenqueue on fatal errors be sufficient?
I think you are right, and the last 2 branches of that if do the same. At a
later point, Sender.runOnce will handle the fatal state and will stop sending,
but maybe it would be cleaner to not reenqueue at this point anymore, yes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
