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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
