-
Notifications
You must be signed in to change notification settings - Fork 54
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
[JUJU-1887] Adds kill delay duration for service shutdown. #190
Conversation
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.
Looks good to me overall. Couple of nit comments and a couple of questions.
ce79b8e
to
899dd30
Compare
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.
Looks good to me now, thank you!
Just passing by to say that's super critical for data platform deployments and we are looking forward to it. 😬 |
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.
Looks very nice, Thomas. Quite a few comments here, but almost all of them are cosmetic. Maybe one item worth pondering on.
6bdd2d5
to
2424889
Compare
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.
All the logic looks fine, thank you.
Only points remaining are about the tuning of times.
shortFailWait = 200 * time.Millisecond | ||
shortOkayDelay = 50 * time.Millisecond | ||
shortKillDelay = 100 * time.Millisecond | ||
shortFailDelay = 100 * time.Millisecond |
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.
Why was this changed?
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.
It's because the original shortFailWait
included both the time from first SIGTERM to SIGKILL and the second wait after that (100+100=200). Now the code is such that "fail delay" only includes that second part (which I think is a bit clearer in any case). So the total wait time here wasn't changed.
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.
Ack, thanks.
- This change adds a new key to the Pebble service plan spec called kill-delay. This allows the author of a service definition to control the amount of time Pebble allows before sending SIGTERM and SIGKILL to a process. - This change also increases the default duration afforded to pebble for kill-delay from 5s to 25s.
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.
Thanks for the collaboration here, everyone.
This change adds a new key to the Pebble service plan spec called kill-delay. This allows the author of a service definition to control the amount of time Pebble allows before sending SIGTERM and SIGKILL to a process.
This change also increases the default duration afforded to pebble for kill-delay from 5s to 25s.