ableegoldman commented on a change in pull request #9487:
URL: https://github.com/apache/kafka/pull/9487#discussion_r525640088
##########
File path: streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java
##########
@@ -932,56 +1028,62 @@ public synchronized boolean close(final long timeout,
final TimeUnit timeUnit) {
return close(timeoutMs);
}
- private boolean close(final long timeoutMs) {
- if (!setState(State.PENDING_SHUTDOWN)) {
- // if transition failed, it means it was either in PENDING_SHUTDOWN
- // or NOT_RUNNING already; just check that all threads have been
stopped
- log.info("Already in the pending shutdown state, wait to complete
shutdown");
- } else {
- stateDirCleaner.shutdownNow();
- if (rocksDBMetricsRecordingService != null) {
- rocksDBMetricsRecordingService.shutdownNow();
- }
+ private Thread shutdownHelper(final boolean error) {
+ stateDirCleaner.shutdownNow();
+ if (rocksDBMetricsRecordingService != null) {
+ rocksDBMetricsRecordingService.shutdownNow();
+ }
- // wait for all threads to join in a separate thread;
- // save the current thread so that if it is a stream thread
- // we don't attempt to join it and cause a deadlock
- final Thread shutdownThread = new Thread(() -> {
- // notify all the threads to stop; avoid deadlocks by stopping
any
- // further state reports from the thread since we're shutting
down
- for (final StreamThread thread : threads) {
- thread.shutdown();
- }
+ // wait for all threads to join in a separate thread;
+ // save the current thread so that if it is a stream thread
+ // we don't attempt to join it and cause a deadlock
+ return new Thread(() -> {
+ // notify all the threads to stop; avoid deadlocks by stopping any
+ // further state reports from the thread since we're shutting down
+ for (final StreamThread thread : threads) {
+ thread.shutdown();
+ }
- for (final StreamThread thread : threads) {
- try {
- if (!thread.isRunning()) {
- thread.join();
- }
- } catch (final InterruptedException ex) {
- Thread.currentThread().interrupt();
+ for (final StreamThread thread : threads) {
+ try {
+ if (!thread.isRunning()) {
+ thread.join();
}
+ } catch (final InterruptedException ex) {
+ Thread.currentThread().interrupt();
}
+ }
- if (globalStreamThread != null) {
- globalStreamThread.shutdown();
- }
+ if (globalStreamThread != null) {
+ globalStreamThread.shutdown();
+ }
Review comment:
Why do we shut down the global thread only after all stream threads have
completed their shutdown? Seems like it would be more efficient to send the
shutdown signal to everyone first, and then wait for all the threads to join.
Can you try this out in the followup 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
