bbejeck commented on code in PR #23791:
URL: https://github.com/apache/kafka/pull/23791#discussion_r4259256469
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/RecordCollectorImpl.java:
##########
@@ -601,13 +601,17 @@ public void closeClean() {
@Override
public void closeDirty() {
log.info("Closing record collector dirty");
-
- if (streamsProducer.eosEnabled()) {
- // We may be closing dirty because the commit failed, so we must
abort the transaction to be safe
- streamsProducer.abortTransaction();
+ try {
+ if (streamsProducer.eosEnabled()) {
+ // We may be closing dirty because the commit failed, so we
must abort the transaction to be safe
+ streamsProducer.abortTransaction();
+ }
+ } catch (final Throwable e) {
+ log.error("Error while aborting transaction during closeDirty", e);
+ throw e;
+ } finally {
+ close();
Review Comment:
I agree with just having the `try/finally` but we never see an error from
the abort exception `close()` executes `TaskManager.executeAndMaybeSwallow` for
`recordCollector.closeDirty()` which catches the exception and issues a debug
log statement. Thinking about this more I don't agree with this and I'll make
the log statement there an `ERROR`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
