Re: [PR] KAFKA-16217: Stop the abort transaction try loop when closing producers [kafka]
jolshan merged PR #15541: URL: https://github.com/apache/kafka/pull/15541 -- 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
Re: [PR] KAFKA-16217: Stop the abort transaction try loop when closing producers [kafka]
jolshan commented on PR #15541: URL: https://github.com/apache/kafka/pull/15541#issuecomment-2027801197 Some failing tests are due to https://issues.apache.org/jira/browse/KAFKA-16447 -- 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
Re: [PR] KAFKA-16217: Stop the abort transaction try loop when closing producers [kafka]
jolshan commented on PR #15541: URL: https://github.com/apache/kafka/pull/15541#issuecomment-2027424479 I don't have further comments. I will restart the build. -- 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
Re: [PR] KAFKA-16217: Stop the abort transaction try loop when closing producers [kafka]
CalvinConfluent commented on PR #15541: URL: https://github.com/apache/kafka/pull/15541#issuecomment-2023173614 @kirktrue @jolshan Anything else we need to address for this ticket? -- 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
Re: [PR] KAFKA-16217: Stop the abort transaction try loop when closing producers [kafka]
kirktrue commented on PR #15541: URL: https://github.com/apache/kafka/pull/15541#issuecomment-2013238826 @CalvinConfluent I wonder if there's any value in the unit test for verifying that `TransactionManager.close()` was invoked. -- 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
Re: [PR] KAFKA-16217: Stop the abort transaction try loop when closing producers [kafka]
kirktrue commented on code in PR #15541:
URL: https://github.com/apache/kafka/pull/15541#discussion_r1529215985
##
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java:
##
@@ -270,14 +270,7 @@ public void run() {
while (!forceClose && transactionManager != null &&
transactionManager.hasOngoingTransaction()) {
if (!transactionManager.isCompleting()) {
log.info("Aborting incomplete transaction due to shutdown");
-
-try {
-// It is possible for the transaction manager to throw
errors when aborting. Catch these
-// so as not to interfere with the rest of the shutdown
logic.
-transactionManager.beginAbort();
-} catch (Exception e) {
-log.error("Error in kafka producer I/O thread while
aborting transaction: ", e);
-}
+transactionManager.beginAbort();
Review Comment:
@jolshan—setting the local `forceClose` flag to `true` looks like it could
work. That would certainly prevent the code from repeating this loop.
That flag being set would then trigger the `transactionManager.close()` call
that occurs after the loop. We'd need to have confidence that the `close()`
logic doesn't get stuck for similar reasons.
--
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
Re: [PR] KAFKA-16217: Stop the abort transaction try loop when closing producers [kafka]
jolshan commented on PR #15541: URL: https://github.com/apache/kafka/pull/15541#issuecomment-200839 Looks pretty reasonable to me. I also want to get a +1 from Kirk to make sure he agrees it makes sense. I will also let the tests run. -- 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
Re: [PR] KAFKA-16217: Stop the abort transaction try loop when closing producers [kafka]
CalvinConfluent commented on PR #15541: URL: https://github.com/apache/kafka/pull/15541#issuecomment-2008340124 @kirktrue Move the test to the Sender test because less mock is needed to repro the bug. -- 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
Re: [PR] KAFKA-16217: Stop the abort transaction try loop when closing producers [kafka]
CalvinConfluent commented on code in PR #15541:
URL: https://github.com/apache/kafka/pull/15541#discussion_r1531252560
##
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java:
##
@@ -270,14 +270,7 @@ public void run() {
while (!forceClose && transactionManager != null &&
transactionManager.hasOngoingTransaction()) {
if (!transactionManager.isCompleting()) {
log.info("Aborting incomplete transaction due to shutdown");
-
-try {
-// It is possible for the transaction manager to throw
errors when aborting. Catch these
-// so as not to interfere with the rest of the shutdown
logic.
-transactionManager.beginAbort();
-} catch (Exception e) {
-log.error("Error in kafka producer I/O thread while
aborting transaction: ", e);
-}
+transactionManager.beginAbort();
Review Comment:
Updated.
--
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
Re: [PR] KAFKA-16217: Stop the abort transaction try loop when closing producers [kafka]
kirktrue commented on PR #15541: URL: https://github.com/apache/kafka/pull/15541#issuecomment-2004890051 @CalvinConfluent—thanks for the PR! This PR doesn't have any unit tests to verify the new behavior. Would it be possible to migrate the test case from your _other PR_ (#15336) to _this PR_? Thanks! -- 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
Re: [PR] KAFKA-16217: Stop the abort transaction try loop when closing producers [kafka]
jolshan commented on code in PR #15541:
URL: https://github.com/apache/kafka/pull/15541#discussion_r1525612166
##
clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java:
##
@@ -270,14 +270,7 @@ public void run() {
while (!forceClose && transactionManager != null &&
transactionManager.hasOngoingTransaction()) {
if (!transactionManager.isCompleting()) {
log.info("Aborting incomplete transaction due to shutdown");
-
-try {
-// It is possible for the transaction manager to throw
errors when aborting. Catch these
-// so as not to interfere with the rest of the shutdown
logic.
-transactionManager.beginAbort();
-} catch (Exception e) {
-log.error("Error in kafka producer I/O thread while
aborting transaction: ", e);
-}
+transactionManager.beginAbort();
Review Comment:
I wonder if we could keep the try catch block, but rather than just log the
error, set forceClose to true
cc: @kirktrue what do you think?
--
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
