Support a configurable delay before shutting down services #2238
Conversation
jdm-square
force-pushed
the
shutdown-delay
branch
2 times, most recently
from
fee1ad6
to
69ce815
Compare
…me before shutting down all services.
jdm-square
force-pushed
the
shutdown-delay
branch
from
69ce815
to
0cb23d3
Compare
| @@ -88,6 +90,10 @@ class MiskApplication(private val modules: List<Module>, commands: List<MiskComm | |||
| Runtime.getRuntime().addShutdownHook(object : Thread() { | |||
| override fun run() { | |||
| log.info { "received a shutdown hook! performing an orderly shutdown" } | |||
| if (injector.getExistingBinding(Key.get(WebConfig::class.java)) != null) { | |||
| val config = injector.getInstance<WebConfig>() | |||
| sleep(config.shutdown_sleep_ms.toLong()) | |||
There was a problem hiding this comment.
This is my attempt to make the delay configurable. It's not clear to me what other avenues exist for making it possible for consumers to customize behaviour at this layer of the framework.
| @@ -88,6 +90,10 @@ class MiskApplication(private val modules: List<Module>, commands: List<MiskComm | |||
| Runtime.getRuntime().addShutdownHook(object : Thread() { | |||
| override fun run() { | |||
| log.info { "received a shutdown hook! performing an orderly shutdown" } | |||
| if (injector.getExistingBinding(Key.get(WebConfig::class.java)) != null) { | |||
There was a problem hiding this comment.
I think this would make a lot more sense at the start of JettyService#shutDown(). Saves having to check with the injector too, and makes the functionality testable (at least, in theory—testing a sleep call probably won't be nice).
There was a problem hiding this comment.
Unfortunately that does not actually help with the problem - the issue is with other services being shut down before the jetty service is finished servicing requests.
There was a problem hiding this comment.
I could move the sleep to the hibernate service instead, I suppose, at the risk of uncovering other services that may also be affected in the same way. Sleeping here avoids the risk of playing whack-a-mole for something that is intended to be a short-term workaround.
This is a stopgap solution for #2105. The correct fix involves supporting explicit dependencies when installing JettyService, as well as modifying the behaviour of ReadinessCheckAction and LivenessCheckAction, and I still intend to work on that. However, this is an issue that is affecting the production readiness of my team's services today, and this change will buy us breathing room.
When misk is used in conjunction with k8s, there's a small window during shutdown in which misk continues to accept new connections before k8s takes the pod out of the rotation. This change makes it possible to configure misk to wait long enough to finish servicing those requests before starting to shut down any services that may be otherwise required. By defaulting to a delay of 0ms, the default behaviour for misk applications is unchanged.