C0urante commented on code in PR #14763:
URL: https://github.com/apache/kafka/pull/14763#discussion_r1424087432
##########
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:
> Do you mean `finally` in `DistributedHerder#run`? `halt` is never called
if `startServices` throws.
Ugh sorry, yes. This is my reward for rushing a review before I close the
laptop for the day!
> 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.
Yeah, this is what I'm afraid of.
Alternatives also include wrapping `startServices` with try/catch logic to
automatically invoke `stopServices` if anything goes wrong (and then proceed to
throw the original exception).
Also, looking at this once more--doesn't the `herderMetrics` field never get
closed in the same scenario that we're addressing with this PR?
--
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]
