gharris1727 commented on code in PR #14763:
URL: https://github.com/apache/kafka/pull/14763#discussion_r1423238556
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java:
##########
@@ -812,6 +812,7 @@ public void stop() {
ThreadUtils.shutdownExecutorServiceQuietly(herderExecutor,
herderExecutorTimeoutMs(), TimeUnit.MILLISECONDS);
ThreadUtils.shutdownExecutorServiceQuietly(forwardRequestExecutor,
FORWARD_REQUEST_SHUTDOWN_TIMEOUT_MS, TimeUnit.MILLISECONDS);
ThreadUtils.shutdownExecutorServiceQuietly(startAndStopExecutor,
START_AND_STOP_SHUTDOWN_TIMEOUT_MS, TimeUnit.MILLISECONDS);
+ stopServices();
Review Comment:
> Would putting stopServices in the finally block in halt achieve the same
thing?
Do you mean `finally` in `DistributedHerder#run`? `halt` is never called if
`startServices` throws.
I considered putting `stopServices` in `catch` in `DistributedHerder#run`,
but I saw that `stopServices` was already called in `StandaloneHerder#stop` and
followed the same pattern.
> I'm a little worried about this exacerbating ungraceful shutdowns in
scenarios other than the one we're trying to address with this change.
For ungraceful shutdowns that end with the herder thread throwing an
exception, it should still call `exit` and kill the process, without ever
running this code.
For ungraceful shutdowns that don't throw an exception, i suppose the herder
could block and `ThreadUtils.shutdownExecutorServiceQuietly(herderExecutor,
...)` would time out, and this `stopServices` would close the KafkaBasedLogs
underneath the running herder, which could be bad.
--
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]
