C0urante commented on code in PR #12290: URL: https://github.com/apache/kafka/pull/12290#discussion_r1425798851
########## connect/runtime/src/test/java/org/apache/kafka/connect/integration/BlockingConnectorTest.java: ########## @@ -139,7 +141,8 @@ public void setup() throws Exception { public void close() { // stop all Connect, Kafka and Zk threads. connect.stop(); - Block.resetBlockLatch(); + // unblock everything so that we don't leak threads after each test run + Block.reset(); Review Comment: > I think that should take place in a test then, not in the cleanup. It's easier to handle in a single method rather than copy over to 11 test cases, though. And I also don't necessarily see why `@After`-annotated classes need to be used exclusively for cleanup. The concern about threads leaking (even if for a short period) beyond the scope of the test definitely seems valid. I've pushed a tweak that adds logic to wait for the blocked threads to complete in `Block::reset`. LMK if this seems clean enough; if not, I can bite the bullet and reverse the order of operations in `BlockingConnectorTest::close` and then see about adding more explicit checks for graceful worker shutdown in other places. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org