fix: adds a handler to gracefully shutdown #5895
Conversation
| @@ -129,12 +129,17 @@ public void startAsync() { | |||
| versionChecker.start(KsqlModuleType.SERVER, properties); | |||
| } catch (final Exception e) { | |||
| log.error("Failed to start KSQL Server with query file: " + queriesFile, e); | |||
| triggerShutdown(); | |||
There was a problem hiding this comment.
Was it a bug that this was here previously, since it means triggerShutdown() was being called from both StandaloneExecutor and KsqlServerMain? (Checking my understanding for why this was removed.)
There was a problem hiding this comment.
I don't think it's a bug necessarily (since triggerShutdown is idempotent). Just not needed.
| // Given: | ||
| final Capture<Runnable> captureShutdownHandler = newCapture(); | ||
| shutdownHandler.execute(capture(captureShutdownHandler)); | ||
| executable.notifyTerminated(); |
There was a problem hiding this comment.
I'm not understanding this test. Are we missing an expectLastCall(); after this line?
|
Let's target this on 6.0.x / 0.10.x and then merge forward to 0.11.x and master. |
Couple changes related to application lifecycle management. Firstly, adds a shutdown handler that gracefully shuts down the service when the jvm determines its time to shut down (e.g. when it receives a termination signal). Secondly, this patch reorganizes some of the startup, steady-state, and shutdown code to make shutdown easier to reason about. Specifically, all these methods are now called from the same thread, so both the thread-safety and order of execution are guaranteed. All the shutdown hook does is notify the main thread and then wait for it to exit.
rodesai
force-pushed
the
fix-ksql-shutdown-hook
branch
from
c36cb53
to
39f91cd
Compare
* fix: adds a handler to gracefully shutdown service Couple changes related to application lifecycle management. Firstly, adds a shutdown handler that gracefully shuts down the service when the jvm determines its time to shut down (e.g. when it receives a termination signal). Secondly, this patch reorganizes some of the startup, steady-state, and shutdown code to make shutdown easier to reason about. Specifically, all these methods are now called from the same thread, so both the thread-safety and order of execution are guaranteed. All the shutdown hook does is notify the main thread and then wait for it to exit.
| @@ -305,7 +304,7 @@ public void startAsync() { | |||
| pullQueryExecutor | |||
| ); | |||
|
|
|||
| startAsyncThread = Thread.currentThread(); | |||
| startAsyncThreadRef.set(Thread.currentThread()); | |||
There was a problem hiding this comment.
Should we use CAS if startAsync is mistakenly triggered multiple times?
There was a problem hiding this comment.
Makes sense. We could move this to be the first thing in the method and then throw if it's ever non-null.
| } | ||
|
|
||
| void tryStartApp() throws Exception { | ||
| final CountDownLatch latch = new CountDownLatch(1); |
There was a problem hiding this comment.
For my own clarification: why we design tryStartApp itself to be blocking until shutdown, instead of letting this function to return after startup procedure completed, and then having another shutdown function that block until shutdown completes?
There was a problem hiding this comment.
I think this might just be my C background talking, but the pattern I've gotten used to is to have the main thread own the application's lifecycle. It starts up all services, waits for a notification to initiate shutdown, and then orchestrates the shutdown process. This pattern makes a great deal of sense in that world because usually you'd write your shutdown in response to a signal, and a signal handler should not block. I think following this pattern in Java makes sense too for the following reasons:
- I wasn't totally sure what constraints the jvm would run the shutdown thread under. It seemed simplest to just have it do as little as possible and let the main thread manage shutdown
- This gives you the guarantee that start is only ever called before shutdown, so it's easier to reason about the two (no synchronization needed, don't need to worry about entering start after calling shutdown).
* fix: adds a handler to gracefully shutdown service Couple changes related to application lifecycle management. Firstly, adds a shutdown handler that gracefully shuts down the service when the jvm determines its time to shut down (e.g. when it receives a termination signal). Secondly, this patch reorganizes some of the startup, steady-state, and shutdown code to make shutdown easier to reason about. Specifically, all these methods are now called from the same thread, so both the thread-safety and order of execution are guaranteed. All the shutdown hook does is notify the main thread and then wait for it to exit.
Description
Couple changes related to application lifecycle management. Firstly, adds a shutdown handler
that gracefully shuts down the service when the jvm determines its time to shut down (e.g.
when it receives a termination signal). Secondly, this patch reorganizes some of the startup,
steady-state, and shutdown code to make shutdown easier to reason about. Specifically, all
these methods are now called from the same thread, so both the thread-safety and order of
execution are guaranteed. All the shutdown hook does is notify the main thread and then wait
for it to exit.
Testing done
unit test to ensure shutdown handler is called
manual testing of shutdown process
Reviewer checklist