[GitHub] [kafka] urbandan commented on a diff in pull request #13591: KAFKA-14831: Illegal state errors should be fatal in transactional producer

["urbandan (via GitHub)" <gi...@apache.org>]

urbandan commented on code in PR #13591:
URL: https://github.com/apache/kafka/pull/13591#discussion_r1170141463


##########
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:
   I think changing the default behavior to not throw can cause issues in some calls:
   1. TransactionManager.InitProducerIdHandler#handleResponse on line 1303 - lastError is explicitly set to null (which shouldn't be done at all, as transitionTo already does that if the state transition is valid), which will clear the latest error. I think to make this work, that lastError = null should be removed from line 1303
   2. This is a call chain where we transition on direct user action, shouldn't this be throwing? KafkaProducer.send -> KafkaProducer.doSend -> maybeTransitionToErrorState -> transitionToAbortableError -> transitionTo
   3. In TransactionManager.TxnOffsetCommitHandler#handleResponse, there are multiple
   ```
   abortableError(...);
   break;
   ```
   blocks. If abortableError does not throw on invalid state transition anymore, the txn commit will be retried, even when in a failed state, which doesn't seem correct.



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org