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

Implement WarningThreshold for TCP/HTTP HealthChecks #4576

Closed

Conversation

pierresouchay
Copy link
Contributor

It implements setting Warning state for HTTP 200 or when TCP
connection succeeds but is too slow as described
in #4522

Combined with #4468 it will
allow very easily to reduce calls/secs on slow nodes in a
completely automatic way.

@pierresouchay pierresouchay force-pushed the WarningThreshold branch 15 times, most recently from 9359dc8 to e7383fe Compare August 28, 2018 23:04
@pierresouchay
Copy link
Contributor Author

\o/ All tests are passing !

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Great work again Pierre.

I've not looked at the changes in too much detail yet as I'm not yet convinced this is the right direction to go for providing load feedback for loadbalancing. See my comment #4522 (comment) for discussion.

@pierresouchay
Copy link
Contributor Author

@banks Ok, let's see this as another way to have a Warning State then.

  • OK => easy => HTTP 200
  • Critical => anything but 429 and 200 => easy
  • Warning => hard in basic script language

On our side, we will probably not use this anyway to achieve such goal, but I had this exact issue for a not maintained anymore PHP script responding after 20 secs on one machine among 3 (because targeted statically by another host while the other 2 were happily answering far less), and I would have loved to be able to fix this that easily.

@ShimmerGlass
Copy link
Contributor

Small detail : we should probably also warn when WarningThreshold > 10s as this the hardcoded HTTP timeout.

@hanshasselberg
Copy link
Member

Closing this PR, discussion here: #4522 (comment).

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.

4 participants