Skip to content

Add graceful shutdown and a shutdown hook in Application.#2

Merged
Ewen Cheslack-Postava (ewencp) merged 1 commit intomasterfrom
shutdown-hook
Jan 7, 2015
Merged

Add graceful shutdown and a shutdown hook in Application.#2
Ewen Cheslack-Postava (ewencp) merged 1 commit intomasterfrom
shutdown-hook

Conversation

@ewencp
Copy link
Copy Markdown
Contributor

The graceful shutdown period, during which new requests are rejected but
outstanding requests are allowed to finish, is configurable. A shutdown hook in
Application also allows additional resources not associated with any single
request to be cleaned up. In order to ensure the process does not exit before
the hook runs, lifecycle methods are added to Application that wrap and augment
the ones in Jetty's Server class.

The graceful shutdown period, during which new requests are rejected but
outstanding requests are allowed to finish, is configurable. A shutdown hook in
Application also allows additional resources not associated with any single
request to be cleaned up. In order to ensure the process does not exit before
the hook runs, lifecycle methods are added to Application that wrap and augment
the ones in Jetty's Server class.
@ewencp
Copy link
Copy Markdown
Contributor Author

Jun Rao (@junrao) probably makes sense for you to review this since you'll want to make sure it suits your needs for schema-registry. I have a version of rest-utils that cleans up some shared state on shutdown using this, so I think this should be good enough. See the example app for the modifications to main() that you need to make since Application now wraps the Jetty Server completely for start/stop.

@ewencp
Copy link
Copy Markdown
Contributor Author

Jun Rao (@junrao) Do you want to look over this to make sure it'll work for schema-registry before I merge? It has some other incidental changes that I need to address another issue, so I'd like to get it committed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make this an abstract method to force the subclasses to define onShutdown()? Most apps probably have some resources that they need to clean up on shutdown.

Other than that, the patch looks good.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that since I agree most apps probably have some resources that should get cleaned up gracefully, but doing so doesn't scale down to tests and simple apps like the example. You end up having to implement methods just to leave them empty. I'm going to leave it as is for now but we can always revisit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants