You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/05/02 00:09:37 UTC

[GitHub] [kafka] hachikuji opened a new pull request #8602: KAFKA-9947; Ensure proper shutdown of components in `TransactionsBounceTest`

hachikuji opened a new pull request #8602:
URL: https://github.com/apache/kafka/pull/8602


   This test case should ensure that clients and the bounce scheduler get shutdown properly. 
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on pull request #8602: KAFKA-9947; Ensure proper shutdown of components in `TransactionsBounceTest`

Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #8602:
URL: https://github.com/apache/kafka/pull/8602#issuecomment-624845577


   Sounds good.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #8602: KAFKA-9947; Ensure proper shutdown of components in `TransactionsBounceTest`

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8602:
URL: https://github.com/apache/kafka/pull/8602#discussion_r419160617



##########
File path: core/src/test/scala/integration/kafka/api/TransactionsBounceTest.scala
##########
@@ -130,13 +129,10 @@ class TransactionsBounceTest extends KafkaServerTestHarness {
         iteration += 1
       }
     } finally {
-      producer.close()
-      consumer.close()

Review comment:
       We are now relying on the lifecycle methods to close the producer and consumer. Did you do that to simplify the code, i.e. it has no impact on the behavior of the test?




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] hachikuji commented on a change in pull request #8602: KAFKA-9947; Ensure proper shutdown of components in `TransactionsBounceTest`

Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #8602:
URL: https://github.com/apache/kafka/pull/8602#discussion_r420948852



##########
File path: core/src/test/scala/integration/kafka/api/TransactionsBounceTest.scala
##########
@@ -156,17 +152,26 @@ class TransactionsBounceTest extends KafkaServerTestHarness {
 
     val expectedValues = (0 until numInputRecords).toSet
     assertEquals(s"Missing messages: ${expectedValues -- recordSet}", expectedValues, recordSet)
+  }
 
-    verifyingConsumer.close()
+  private def createTransactionalProducer(transactionalId: String) = {
+    val props = new Properties()
+    props.put(ProducerConfig.ACKS_CONFIG, "all")
+    props.put(ProducerConfig.BATCH_SIZE_CONFIG, "512")
+    props.put(ProducerConfig.TRANSACTIONAL_ID_CONFIG, transactionalId)
+    props.put(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "true")
+    createProducer(configOverrides = props)

Review comment:
       Nice catch. Yeah, let me fix that.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] hachikuji commented on a change in pull request #8602: KAFKA-9947; Ensure proper shutdown of components in `TransactionsBounceTest`

Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #8602:
URL: https://github.com/apache/kafka/pull/8602#discussion_r419595855



##########
File path: core/src/test/scala/integration/kafka/api/TransactionsBounceTest.scala
##########
@@ -130,13 +129,10 @@ class TransactionsBounceTest extends KafkaServerTestHarness {
         iteration += 1
       }
     } finally {
-      producer.close()
-      consumer.close()

Review comment:
       Yeah, I thought it is a preferred pattern to simplify cleanup a little bit in test cases. It does not change the behavior of the test.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] hachikuji commented on pull request #8602: KAFKA-9947; Ensure proper shutdown of components in `TransactionsBounceTest`

Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #8602:
URL: https://github.com/apache/kafka/pull/8602#issuecomment-624842318


   I changed the patch to set the request timeout the same as the delivery timeout for this test case, which was the old behavior.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [kafka] ijuma commented on a change in pull request #8602: KAFKA-9947; Ensure proper shutdown of components in `TransactionsBounceTest`

Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8602:
URL: https://github.com/apache/kafka/pull/8602#discussion_r420942672



##########
File path: core/src/test/scala/integration/kafka/api/TransactionsBounceTest.scala
##########
@@ -156,17 +152,26 @@ class TransactionsBounceTest extends KafkaServerTestHarness {
 
     val expectedValues = (0 until numInputRecords).toSet
     assertEquals(s"Missing messages: ${expectedValues -- recordSet}", expectedValues, recordSet)
+  }
 
-    verifyingConsumer.close()
+  private def createTransactionalProducer(transactionalId: String) = {
+    val props = new Properties()
+    props.put(ProducerConfig.ACKS_CONFIG, "all")
+    props.put(ProducerConfig.BATCH_SIZE_CONFIG, "512")
+    props.put(ProducerConfig.TRANSACTIONAL_ID_CONFIG, transactionalId)
+    props.put(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "true")
+    createProducer(configOverrides = props)

Review comment:
       I was checking the difference between what we do now and before and noticed that we have this line in the helper method we were using previously:
   
   > props.put(ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG, deliveryTimeoutMs.toString)
   
   That seems a bit questionable. Should we remove it from that method?




----------------------------------------------------------------
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:
users@infra.apache.org