-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Add docs for CHECKS #849
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 docs for CHECKS #849
Conversation
|
Actually, I think those docs are wrong. It seems as though we always wait at least 60 seconds before killing old containers. @michaelshobbs does that seem right ? |
|
Ah sorry, I read your docs wrong :( |
docs/application-deployment.md
Outdated
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.
This is what I meant was false. We definitely wait at least 60 seconds - configurable - before killing old containers.
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.
Maybe my explanation wasn't clear. Although we keep the old container around, new traffic is sent to the new container right away.
This can lead to 502s if your app doesn't bind to the port right away, an easy way to repro this is using a setTimeout around the listen call in Heroku's sample node.js app.
Maybe this is an edge-case and not worth bringing up in the docs.
Thoughts?
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.
@josegonzalez what are you saying is false?
The sentence regarding DOKKU_CHECKS_WAIT is true. We should also add a mention of DOKKU_WAIT_TO_RETIRE as that is was controls how long the old container has to fulfill in-flight requests before being killed.
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.
There's a short wait for the container to start, then the checks are run against it, before switching traffic. So if the new container is not able to handle request, traffic keeps flowing to the old one, new container gets destroyed.
The long timeout servers two purposes. First, some responses are just slow. Second long lasting connections (eg server sent events), helps to gradually migrate those over to new server.
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 other than my comment. Just remove that line and we should be good to merge :) |
|
Yup 60 is the default. |
b77b258 to
7bd5df0
Compare
|
I removed the line about empty This got me testing Still waits 60 seconds: I'll open a separate issue for this. |
Fixes #693
The docs are based on #562 and @assaf's blog post.
Review
@josegonzalez @michaelshobbs
cc @travischoma