-
Notifications
You must be signed in to change notification settings - Fork 921
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
Allow XdsEndpointGroup
to disable health checking per Endpoint
#5879
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.
👍 👍 👍
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 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.
Overall, looks good. Left minor comments.
.../java/com/linecorp/armeria/internal/client/endpoint/healthcheck/StaticHttpHealthChecker.java
Outdated
Show resolved
Hide resolved
ClientRequestContext.of(HttpRequest.of(HttpMethod.GET, "/")); | ||
|
||
private StaticHttpHealthChecker(HealthCheckerContext ctx, double healthy) { | ||
ctx.updateHealth(healthy, NOOP_CTX, null, null); |
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.
How about specifying null
instead of a mock context? I think that would be more clear.
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.
Modified the contract of updateHealth
to accept a @Nullable
9aa9249
to
154ea0f
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.
Nice work!
Motivation:
While working on documentation, I realized that
disable_active_health_check
is not supported.ref: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/endpoint/v3/endpoint_components.proto#config-endpoint-v3-endpoint-healthcheckconfig
This can be useful when users would like to force certain endpoints to be healthy without health checking.
Modifications:
HttpHealthChecker
toDefaultHttpHealthChecker
HttpHealthChecker
interfaceStaticHttpHealthChecker
which sets the health status immediatelyHealthCheckConfig#getDisableActiveHealthCheck
and decide whether to return aStaticHttpHealthChecker
Result:
disable_active_health_check
is now supported