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
