MINOR: Rewrite ClientOAuthIntegrationTest from Scala to Java#22520
MINOR: Rewrite ClientOAuthIntegrationTest from Scala to Java#22520m1a2st wants to merge 9 commits into
Conversation
github-actions
Bot
added
core
m1a2st
force-pushed
the
MINOR-migration-ClientOAuthIntegrationTest
branch
from
aca2c65 to
7fe8b5f
Compare
| private static final String ATTACH_LISTENER_THREAD_PREFIX = "Attach Listener"; | ||
| private static final String PROCESS_REAPER_THREAD_PREFIX = "process reaper"; | ||
| private static final String RMI_THREAD_PREFIX = "RMI"; | ||
| private static final String MOCK_WEB_SERVER_THREAD_PREFIX = "MockWebServer"; |
There was a problem hiding this comment.
Would you mind sharing the reasoning with us?
There was a problem hiding this comment.
MockOAuth2Server library internally uses OkHttp's MockWebServer, which spawns threads with the "MockWebServer" prefix. These threads may not be fully terminated by the time the thread leak check runs after each test, causing false-positive thread leak detection failures.
There was a problem hiding this comment.
Is it an existing issue from before? Did you observe it just because the new framework has stricter constraints?
There was a problem hiding this comment.
Yes, this is an existing issue. The original Scala version extended IntegrationTestHarness which does not have thread leak detection. After migrating to the new @ClusterTest framework, the ClusterTestExtensions automatically performs thread leak checking in afterEach(), which exposed the MockWebServer threads that were not fully terminated after mockOAuthServer.shutdown(). So the leak was always there, but the stricter constraints of the new framework made it visible.
There was a problem hiding this comment.
Thanks for the explanation. Would you mind adding a comment for it? It would be good to point out which test spawn those threads.
There was a problem hiding this comment.
Sure, addressed it!
| // internally) in ClientOAuthIntegrationTest. | ||
| private static final String MOCK_WEB_SERVER_THREAD_PREFIX = "MockWebServer"; | ||
| private static final String KEEP_ALIVE_TIMER_THREAD_PREFIX = "Keep-Alive-Timer"; | ||
| private static final String POOL_THREAD_PREFIX = "pool-"; |
There was a problem hiding this comment.
pool- is a bit too common I guess. Would you mind using a more specific name instead?
There was a problem hiding this comment.
I tried to find a more specific name, but the leaked threads are named pool-N-thread-M, which is the default name produced by Executors.defaultThreadFactory().
There was a problem hiding this comment.
Executors.defaultThreadFactory().
Could you share the code with me? It looks like a real leak?
There was a problem hiding this comment.
The test error log is
Thread leak detected: pool-11-thread-1, pool-10-thread-1, pool-8-thread-1
Expected :true
Actual :false
<Click to see difference>
org.opentest4j.AssertionFailedError: Thread leak detected: pool-11-thread-1, pool-10-thread-1, pool-8-thread-1 ==> expected: <true> but was: <false>
at org.apache.kafka.common.test.junit.ClusterTestExtensions.afterEach(ClusterTestExtensions.java:192)
at java.base/java.util.Optional.ifPresent(Optional.java:178)
at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708)
at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
# Conflicts: # core/src/test/scala/integration/kafka/api/ClientOAuthIntegrationTest.scala # test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/junit/ClusterTestExtensions.java
| threads.stream().map(t -> { | ||
| return t.getThreadGroup().getName() + "/" + t.getName(); | ||
| }).collect(Collectors.joining(", "))); | ||
| threads.stream().map(Thread::getName).collect(Collectors.joining(", "))); |
There was a problem hiding this comment.
This is unrelated to this patch. I have reverted it.
2 participants
Rewite ClientOAuthIntegrationTest and move to clients-integration-tests
module
Reviewers: Chia-Ping Tsai chia7712@gmail.com