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/08/19 00:06:21 UTC
[GitHub] [kafka] lbradstreet opened a new pull request #9200: MINOR: mirror integration tests should not call System.exit
lbradstreet opened a new pull request #9200:
URL: https://github.com/apache/kafka/pull/9200
406635bcc9f2a4c439d198ea0549170de331323c switched System.exit to using Exit.exit, however the integration tests did not choose to override the exit procedure.
This can cause invalid test runs with errors like this:
```
[2020-08-18T15:52:01.142Z] * What went wrong:
[2020-08-18T15:52:01.142Z] Execution failed for task ':connect:mirror:integrationTest'.
[2020-08-18T15:52:01.142Z] > Process 'Gradle Test Executor 192' finished with non-zero exit value 1
[2020-08-18T15:52:01.142Z] This problem might be caused by incorrect test process configuration.
[2020-08-18T15:52:01.142Z] Please refer to the test execution section in the User Manual at https://docs.gradle.org/6.6/userguide/java_testing.html#sec:test_execution
```
----------------------------------------------------------------
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] C0urante commented on pull request #9200: MINOR: mirror integration tests should not call System.exit
Posted by GitBox <gi...@apache.org>.
C0urante commented on pull request #9200:
URL: https://github.com/apache/kafka/pull/9200#issuecomment-737550229
@lbradstreet I think the changes to the MM2 tests may have been unnecessary or, at least, inadequate. Embedded Connect clusters automatically [mask exit procedures on startup](https://github.com/apache/kafka/blob/aeeb7b2f9a9abe8f49543a2278757722e5974cb3/connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedConnectCluster.java#L128-L138) by default, which should apply to the MM2 integration tests.
I believe the issue with the MM2 integration tests is that embedded Connect clusters also automatically [reset exit procedures on shutdown](https://github.com/apache/kafka/blob/aeeb7b2f9a9abe8f49543a2278757722e5974cb3/connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedConnectCluster.java#L156-L157). Since the tests use two embedded Connect clusters which are shut down sequentially at the end of each test case, the second cluster to be shut down does so without using the masked exit procedures, which can result in `System::exit` being 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rajinisivaram commented on pull request #9200: MINOR: mirror integration tests should not call System.exit
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on pull request #9200:
URL: https://github.com/apache/kafka/pull/9200#issuecomment-689624548
@lbradstreet Thanks for the PR, merging to trunk.
----------------------------------------------------------------
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] omkreddy commented on pull request #9200: MINOR: mirror integration tests should not call System.exit
Posted by GitBox <gi...@apache.org>.
omkreddy commented on pull request #9200:
URL: https://github.com/apache/kafka/pull/9200#issuecomment-675864751
ok to 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] ijuma commented on pull request #9200: MINOR: mirror integration tests should not call System.exit
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9200:
URL: https://github.com/apache/kafka/pull/9200#issuecomment-678666729
Ok to 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] lbradstreet commented on a change in pull request #9200: MINOR: mirror integration tests should not call System.exit
Posted by GitBox <gi...@apache.org>.
lbradstreet commented on a change in pull request #9200:
URL: https://github.com/apache/kafka/pull/9200#discussion_r473110543
##########
File path: connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorConnectorsIntegrationTest.java
##########
@@ -185,10 +192,10 @@ public void close() {
deleteAllTopics(backup.kafka());
primary.stop();
backup.stop();
+ assertFalse(exited.get());
+ Exit.resetExitProcedure();
Review comment:
I've pushed a fix. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rajinisivaram commented on a change in pull request #9200: MINOR: mirror integration tests should not call System.exit
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on a change in pull request #9200:
URL: https://github.com/apache/kafka/pull/9200#discussion_r472885774
##########
File path: connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorConnectorsIntegrationTest.java
##########
@@ -185,10 +192,10 @@ public void close() {
deleteAllTopics(backup.kafka());
primary.stop();
backup.stop();
+ assertFalse(exited.get());
+ Exit.resetExitProcedure();
Review comment:
I am guessing we ran into an issue because of exit, so perhaps this reset ought to be in a finally block?
----------------------------------------------------------------
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] lbradstreet commented on a change in pull request #9200: MINOR: mirror integration tests should not call System.exit
Posted by GitBox <gi...@apache.org>.
lbradstreet commented on a change in pull request #9200:
URL: https://github.com/apache/kafka/pull/9200#discussion_r472723313
##########
File path: connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorConnectorsIntegrationTest.java
##########
@@ -185,10 +192,9 @@ public void close() {
deleteAllTopics(backup.kafka());
primary.stop();
backup.stop();
+ assertFalse(exited.get());
Review comment:
Ah, fancy GitHub stuff. I pushed up the fix independently.
----------------------------------------------------------------
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] lbradstreet commented on a change in pull request #9200: MINOR: mirror integration tests should not call System.exit
Posted by GitBox <gi...@apache.org>.
lbradstreet commented on a change in pull request #9200:
URL: https://github.com/apache/kafka/pull/9200#discussion_r473075940
##########
File path: connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorConnectorsIntegrationTest.java
##########
@@ -185,10 +192,10 @@ public void close() {
deleteAllTopics(backup.kafka());
primary.stop();
backup.stop();
+ assertFalse(exited.get());
+ Exit.resetExitProcedure();
Review comment:
@rajinisivaram good point. I'll 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] lbradstreet commented on a change in pull request #9200: MINOR: mirror integration tests should not call System.exit
Posted by GitBox <gi...@apache.org>.
lbradstreet commented on a change in pull request #9200:
URL: https://github.com/apache/kafka/pull/9200#discussion_r472723414
##########
File path: core/src/test/scala/integration/kafka/tools/MirrorMakerIntegrationTest.scala
##########
@@ -29,14 +29,31 @@ import org.apache.kafka.clients.producer.{ProducerConfig, ProducerRecord}
import org.apache.kafka.common.TopicPartition
import org.apache.kafka.common.errors.TimeoutException
import org.apache.kafka.common.serialization.{ByteArrayDeserializer, ByteArraySerializer}
+import org.apache.kafka.common.utils.Exit
+import org.junit.After
import org.junit.Test
import org.junit.Assert._
+import org.junit.Before
class MirrorMakerIntegrationTest extends KafkaServerTestHarness {
override def generateConfigs: Seq[KafkaConfig] =
TestUtils.createBrokerConfigs(1, zkConnect).map(KafkaConfig.fromProps(_, new Properties()))
+ val exited = new AtomicBoolean(false)
+
+ @Before
+ override def setUp(): Unit = {
+ Exit.setExitProcedure((_, _) => exited.set(true))
+ super.setUp()
+ }
+
+ @After
+ override def tearDown(): Unit = {
+ super.tearDown()
+ assertFalse(exited.get())
Review comment:
Done
----------------------------------------------------------------
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] rondagostino commented on a change in pull request #9200: MINOR: mirror integration tests should not call System.exit
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #9200:
URL: https://github.com/apache/kafka/pull/9200#discussion_r472636924
##########
File path: connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorConnectorsIntegrationTest.java
##########
@@ -185,10 +192,9 @@ public void close() {
deleteAllTopics(backup.kafka());
primary.stop();
backup.stop();
+ assertFalse(exited.get());
Review comment:
```suggestion
assertFalse(exited.get());
Exit.resetExitProcedure();
```
##########
File path: core/src/test/scala/integration/kafka/tools/MirrorMakerIntegrationTest.scala
##########
@@ -29,14 +29,31 @@ import org.apache.kafka.clients.producer.{ProducerConfig, ProducerRecord}
import org.apache.kafka.common.TopicPartition
import org.apache.kafka.common.errors.TimeoutException
import org.apache.kafka.common.serialization.{ByteArrayDeserializer, ByteArraySerializer}
+import org.apache.kafka.common.utils.Exit
+import org.junit.After
import org.junit.Test
import org.junit.Assert._
+import org.junit.Before
class MirrorMakerIntegrationTest extends KafkaServerTestHarness {
override def generateConfigs: Seq[KafkaConfig] =
TestUtils.createBrokerConfigs(1, zkConnect).map(KafkaConfig.fromProps(_, new Properties()))
+ val exited = new AtomicBoolean(false)
+
+ @Before
+ override def setUp(): Unit = {
+ Exit.setExitProcedure((_, _) => exited.set(true))
+ super.setUp()
+ }
+
+ @After
+ override def tearDown(): Unit = {
+ super.tearDown()
+ assertFalse(exited.get())
Review comment:
```suggestion
assertFalse(exited.get())
Exit.resetExitProcedure()
```
----------------------------------------------------------------
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] rajinisivaram commented on pull request #9200: MINOR: mirror integration tests should not call System.exit
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on pull request #9200:
URL: https://github.com/apache/kafka/pull/9200#issuecomment-679249066
ok to 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] rajinisivaram merged pull request #9200: MINOR: mirror integration tests should not call System.exit
Posted by GitBox <gi...@apache.org>.
rajinisivaram merged pull request #9200:
URL: https://github.com/apache/kafka/pull/9200
----------------------------------------------------------------
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