cmccabe commented on a change in pull request #10705: URL: https://github.com/apache/kafka/pull/10705#discussion_r633914096
########## File path: metadata/src/test/java/org/apache/kafka/metalog/LocalLogManager.java ########## @@ -331,22 +343,40 @@ public void beginShutdown() { } @Override - public void close() throws InterruptedException { + public void close() { log.debug("Node {}: closing.", nodeId); beginShutdown(); - eventQueue.close(); + + try { + eventQueue.close(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException(e); + } + } + + @Override + public CompletableFuture<Void> shutdown(int timeoutMs) { Review comment: It seems like the intention behind this `shutdown` API is to be non-blocking, but this implementation is not non-blocking. Maybe it would be good to add a comment about this? In general it is not possible to do a non-blocking thread join unless you have a third thread (not the calling thread, not the thread being shut down) which can wait for the blocking thread join operation to complete and then complete a future (or whatever). That's why there are two shutdown APIs in LocalLogManager: a non-blocking beginShutdown and a blocking close which does all that, plus the thread join. This is a pattern that I use in other places as well. I think it's more useful than returning a future from close, due to the problem I mentioned above. It could be worth considering for RaftClient in the future. -- 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: us...@infra.apache.org