New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Shutdown method to registry.Registry #4338
Conversation
976913f
to
a34e501
Compare
Missed some really obvious things. Fixes are pushed, and my own fork is working nicely in my embedding app. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no reason why we shouldn't accept this change. LGTM. PTAL @thaJeztah
Looks like there's some fix-up commits in the PR that should be squashed; can you clean up commit history a bit? |
registry/registry.go
Outdated
func (registry *Registry) Shutdown() error { | ||
dcontext.GetLogger(registry.app).Info("stopping server gracefully. Draining connections for ", registry.config.HTTP.DrainTimeout) | ||
// shutdown the server with a grace period of configured timeout | ||
c, cancel := context.WithTimeout(context.Background(), registry.config.HTTP.DrainTimeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is new functionality; would it make sense to pass a context to the function; also to make it closer to the server.Shutdown()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified to function to accept a context instead of setting its own. Please take a look.
d4a3696
to
dda410a
Compare
Signed-off-by: Robin Ketelbuters <robin.ketelbuters@gmail.com>
dda410a
to
16a305e
Compare
Sorry, I'm used to squashing my PRs. I just did so. |
Signed-off-by: David Karlsson <35727626+dvdksn@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
Adds a Shutdown method to
registry.Registry
. Enables gracefully shutting down an embedded registry HTTP server.Closes #4337.