Skip to content
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 --max-time to the curl health check. #5

Merged
merged 2 commits into from Nov 7, 2018
Merged

Add --max-time to the curl health check. #5

merged 2 commits into from Nov 7, 2018

Conversation

mjkillough
Copy link
Contributor

We're seeing the application accept connections but then hang. This
changes the curl command to time out the request after 5s (default ECS
health check timeout) which will cause it to retry.

It's unclear whether this is a debugging step or a workaround. Will test
in staging initially.

We're seeing the application accept connections but then hang. This
changes the curl command to time out the request after 5s (default ECS
health check timeout) which will cause it to retry.

It's unclear whether this is a debugging step or a workaround. Will test
in staging initially.
Copy link
Contributor

@edds edds left a comment

Choose a reason for hiding this comment

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

This seems legitimate as if the health check takes longer than that it's not healthy. But equally I agree it should be a debugging step to work out why it's taking that long. Our current theory with orderweb deploys is getting a postgres connection is causing these timeouts.

@mjkillough
Copy link
Contributor Author

@edds - We now know that this doesn't help. I'm still inclined to merge it, as having curl wait forever doesn't seem useful. What do you think?

@edds
Copy link
Contributor

edds commented Nov 7, 2018

I think it still adds value. Merge away.

@mjkillough mjkillough merged commit 7479e10 into master Nov 7, 2018
@mjkillough mjkillough deleted the timeout branch November 7, 2018 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants