[GitHub] [kafka] hachikuji commented on a change in pull request #8782: KAFKA-10080; Fix race condition on txn completion which can cause duplicate appends

[GitBox]

hachikuji commented on a change in pull request #8782:
URL: https://github.com/apache/kafka/pull/8782#discussion_r434306556



##
File path: 
core/src/test/scala/unit/kafka/coordinator/transaction/TransactionMarkerChannelManagerTest.scala
##
@@ -86,6 +90,70 @@ class TransactionMarkerChannelManagerTest {
   .anyTimes()
   }
 
+  @Test
+  def shouldOnlyWriteTxnCompletionOnce(): Unit = {

Review comment:
   It fails deterministically without the fix.





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.

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

[GitHub] [kafka] hachikuji commented on a change in pull request #8782: KAFKA-10080; Fix race condition on txn completion which can cause duplicate appends

[GitBox]

hachikuji commented on a change in pull request #8782:
URL: https://github.com/apache/kafka/pull/8782#discussion_r434218326



##
File path: 
core/src/test/scala/unit/kafka/coordinator/transaction/TransactionMarkerChannelManagerTest.scala
##
@@ -86,6 +90,70 @@ class TransactionMarkerChannelManagerTest {
   .anyTimes()
   }
 
+  @Test
+  def shouldOnlyWriteTxnCompletionOnce(): Unit = {

Review comment:
   It does. I was trying to setup this test to fit how we're likely hitting 
this in practice. In the call to `addTxnMarkersToSend`, before calling 
`maybeWriteTxnCompletion`, we have to acquire the lock. It is possible that the 
caller fails to acquire the lock before the markers finish getting written and 
the transaction gets completed in the request completion handler.





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.

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

[GitHub] [kafka] hachikuji commented on a change in pull request #8782: KAFKA-10080; Fix race condition on txn completion which can cause duplicate appends

[GitBox]

hachikuji commented on a change in pull request #8782:
URL: https://github.com/apache/kafka/pull/8782#discussion_r434153210



##
File path: 
core/src/test/scala/unit/kafka/coordinator/transaction/TransactionMarkerChannelManagerTest.scala
##
@@ -221,7 +221,6 @@ class TransactionMarkerChannelManagerTest {
 EasyMock.replay(metadataCache)
 
 channelManager.addTxnMarkersToSend(coordinatorEpoch, txnResult, 
txnMetadata1, txnMetadata1.prepareComplete(time.milliseconds()))
-channelManager.addTxnMarkersToSend(coordinatorEpoch, txnResult, 
txnMetadata2, txnMetadata2.prepareComplete(time.milliseconds()))

Review comment:
   The change to use `mock` instead of `niceMock` led to a failure because 
of an unexpected append to the log. It seemed like this call was not necessary 
to test the behavior we were interested in here, so I removed it rather than 
adding the expected call to append.





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.

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

[GitHub] [kafka] hachikuji commented on a change in pull request #8782: KAFKA-10080; Fix race condition on txn completion which can cause duplicate appends

[GitBox]

hachikuji commented on a change in pull request #8782:
URL: https://github.com/apache/kafka/pull/8782#discussion_r434098545



##
File path: 
core/src/main/scala/kafka/coordinator/transaction/TransactionMarkerChannelManager.scala
##
@@ -285,15 +280,16 @@ class TransactionMarkerChannelManager(config: KafkaConfig,
 }
   }
 
-  private def maybeWriteTxnCompletion(transactionalId: String): Unit = {
-Option(transactionsWithPendingMarkers.get(transactionalId)).foreach { 
pendingCommitTxn =>

Review comment:
   Multiple threads may see the transaction still as pending and attempt 
completion.





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.

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