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 2021/01/21 02:18:15 UTC
[GitHub] [kafka] showuon opened a new pull request #9942: KAFKA-12229: reset to original class loader after connector stop
showuon opened a new pull request #9942:
URL: https://github.com/apache/kafka/pull/9942
```
java.lang.NullPointerException at org.apache.kafka.connect.mirror.MirrorSourceConnector.listTopics(MirrorSourceConnector.java:348) at org.apache.kafka.connect.mirror.MirrorSourceConnector.findSourceTopicPartitions(MirrorSourceConnector.java:192) at org.apache.kafka.connect.mirror.MirrorSourceConnectorTest.testRefreshTopicPartitionsTopicOnTargetFirst(MirrorSourceConnectorTest.java:222)
```
After days of investigation, I finally found the root cause of the test failure reason: **class loader**.
The issue is quite weird, we mocked the method, but still call the real method, and cause the NPE. Digging into the Mockito, found it's not about JUnit 5, it's because of the class loader. In Mockito, we relies on the class loader to generate the proxy instance ([source](https://github.com/mockito/mockito/blob/release/3.x/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java#L91)), and if the class loader is not expected, we'll generate the wrong proxy instance (with wrong class path). We set the class loader during connector start to resolve conflicting dependencies (KIP-146), so we should set it back to the original class loader after connector stop in tests (`EmbeddedConnectCluster` is only used in tests) for Mockito works as expected.
### 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] showuon commented on pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#issuecomment-764465027
All tests passed.
----------------------------------------------------------------
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] kkonstantine commented on a change in pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on a change in pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#discussion_r564046001
##########
File path: connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedConnectCluster.java
##########
@@ -157,6 +160,7 @@ public void stop() {
Exit.resetExitProcedure();
Exit.resetHaltProcedure();
}
+ Thread.currentThread().setContextClassLoader(originalClassLoader);
Review comment:
```suggestion
Plugins.compareAndSwapLoaders(originalClassLoader);
```
----------------------------------------------------------------
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] kkonstantine merged pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
kkonstantine merged pull request #9942:
URL: https://github.com/apache/kafka/pull/9942
----------------------------------------------------------------
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] kkonstantine commented on pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#issuecomment-767272618
Indeed @showuon
jdk8 and 15 are green. Thanks for the non-obvious fix.
LGTM
----------------------------------------------------------------
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] showuon commented on pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#issuecomment-766587188
So, to easily reproduce it, you can try to start the `EmbeddedConnectCluster` before the tests in `MirrorSourceConnectorTest`. What I did is to copy the beforeEach `startClusters()` method from `MirrorConnectorsIntegrationBaseTest` and paste into the `MirrorSourceConnectorTest`. So you'll find even we mock the method, the real methods still got called.
----------------------------------------------------------------
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] showuon edited a comment on pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#issuecomment-766574155
@kkonstantine , thanks for the comments. Answer your questions below:
1. Have you noticed how often that happens (every time or occasionally)?
--> I'd say, it happens **often**. I just checked the _kafka-trunk-jdk11_, it failed the test cases from build #423 ~ # 431 (the latest). And also happens a lot in _kafka-trunk-jdk8_ and _kafka-trunk-jdk15_. And sometimes it didn't happen is because the unit tests run faster than the integration tests.
2. Are you saying there's an interference of integration tests with unit tests when Connect integration tests run before the MM2 unit tests? If that's the case, I think this connection is missing from the description.
--> Yes, you're right. There's an interference of integration tests with unit tests when Connect integration tests run before the MM2 unit tests. I've added in the description.
Thank you very much.
----------------------------------------------------------------
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] showuon commented on pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#issuecomment-766574155
@kkonstantine , thanks for the comments. Answer your questions below:
1. Have you noticed how often that happens (every time or occasionally)?
--> I'd say, it happens **often**. I just checked the _kafka-trunk-jdk11_, it failed the test cases from build #423 ~ # 431 (the latest). And also happens a lot in _kafka-trunk-jdk8_ and _kafka-trunk-jdk15_.
2. Are you saying there's an interference of integration tests with unit tests when Connect integration tests run before the MM2 unit tests? If that's the case, I think this connection is missing from the description.
--> Yes, you're right. There's an interference of integration tests with unit tests when Connect integration tests run before the MM2 unit tests. I've added in the description.
Thank you very much.
----------------------------------------------------------------
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] kkonstantine commented on pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#issuecomment-767272618
Indeed @showuon
jdk8 and 15 are green. Thanks for the non-obvious fix.
LGTM
----------------------------------------------------------------
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] showuon commented on pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#issuecomment-766495717
@chia7712 @ijuma , please help review 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#issuecomment-764189012
----------------------------------------------------------------
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] showuon edited a comment on pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#issuecomment-766574155
@kkonstantine , thanks for the comments. Answer your questions below:
1. Have you noticed how often that happens (every time or occasionally)?
--> I'd say, it happens **often**. I just checked the _kafka-trunk-jdk11_, it failed the test cases from build #423 ~ # 431 (the latest). And also happens a lot in _kafka-trunk-jdk8_ and _kafka-trunk-jdk15_. And sometimes it didn't happen is because the unit tests run faster than the integration tests.
2. Are you saying there's an interference of integration tests with unit tests when Connect integration tests run before the MM2 unit tests? If that's the case, I think this connection is missing from the description.
--> Yes, you're right. There's an interference of integration tests with unit tests when Connect integration tests run before the MM2 unit tests. I've added in the description.
Thank you very much.
----------------------------------------------------------------
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] kkonstantine commented on a change in pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on a change in pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#discussion_r564046001
##########
File path: connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedConnectCluster.java
##########
@@ -157,6 +160,7 @@ public void stop() {
Exit.resetExitProcedure();
Exit.resetHaltProcedure();
}
+ Thread.currentThread().setContextClassLoader(originalClassLoader);
Review comment:
```suggestion
Plugins.compareAndSwapLoaders(originalClassLoader);
```
----------------------------------------------------------------
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] showuon commented on pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#issuecomment-767424805
The latest _kafka-trunk-jdk8_, _kafka-trunk-jdk11_, _kafka-trunk-jdk15_ are not seeing these failed tests anymore. YEAH~
----------------------------------------------------------------
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] showuon commented on pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#issuecomment-766495717
----------------------------------------------------------------
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] showuon commented on pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#issuecomment-764189012
@chia7712 @ijuma @guozhangwang , please take a look at the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9942:
URL: https://github.com/apache/kafka/pull/9942#issuecomment-767263182
@kkonstantine , thanks for the comments and updates.
Failed tests are not related to my changes.
```
Test Result (2 failures / +2)
Build / JDK 11 / org.apache.kafka.clients.consumer.KafkaConsumerTest.testCloseWithTimeUnit()
Build / JDK 11 / org.apache.kafka.clients.consumer.internals.FetcherTest.testEarlierOffsetResetArrivesLate()
```
----------------------------------------------------------------
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] kkonstantine merged pull request #9942: KAFKA-12229: reset to original class loader after connector stop
Posted by GitBox <gi...@apache.org>.
kkonstantine merged pull request #9942:
URL: https://github.com/apache/kafka/pull/9942
----------------------------------------------------------------
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