Add GrpcHealthCheckedEndpointGroupBuilder#6078
Conversation
Motivation: Add `GrpcHealthCheckedEndpointGroupBuilder` which builds a health checked endpoint group whose health comes from a [standard gRPC health check service result](https://grpc.io/docs/guides/health-checking/). Modifications: * Adds `GrpcHealthCheckedEndpointGroupBuilder` which extends `AbstractHealthCheckedEndpointGroupBuilder` and creates a new health check function * Adds `GrpcHealthChecker` which is the health check function that creates and uses a gRPC `HealthGrpc` stub to check the gRPC health service on the endpoint. If the health check response is `SERVING`, it is healthy. It is unhealthy if the response is not `SERVING` or if there was a request failure. * Adds tests. Result: * A user can create a health checked endpoint group that is backed by a gRPC health check service. * Closes line#5930
|
|
| if (healthCheckResponse.getStatus() == HealthCheckResponse.ServingStatus.SERVING) { | ||
| ctx.updateHealth(HEALTHY, reqCtx, null, null); | ||
| } else { | ||
| // not sure about the response headers but it needs to be non-null |
There was a problem hiding this comment.
I think it's fine to leave the headers as null for now.
Alternatively, we can also extract the headers from the ctx log
ResponseHeaders responseHeaders = null;
if (reqCtx.log().isAvailable(RequestLogProperty.RESPONSE_HEADERS)) {
responseHeaders = reqCtx.log().partial().responseHeaders();
}| lock(); | ||
| try { | ||
| final HealthCheckRequest.Builder builder = HealthCheckRequest.newBuilder(); | ||
| if (this.service != null) { |
There was a problem hiding this comment.
| if (this.service != null) { | |
| if (service != null) { |
| public void onError(Throwable throwable) { | ||
| final ClientRequestContext reqCtx = reqCtxCaptor.get(); | ||
| // same here | ||
| ctx.updateHealth(UNHEALTHY, reqCtx, UNHEALTHY_RESPONSE_HEADERS, throwable); | ||
| } | ||
|
|
||
| @Override | ||
| public void onCompleted() { | ||
| } |
There was a problem hiding this comment.
Question) If the connection is dropped, is there no need to retry the check (with a backoff)?
| } | ||
|
|
||
| public void start() { | ||
| check(); |
There was a problem hiding this comment.
Question) Is there no need to also implement watch for completeness? Also, what do you think of allowing users to configure which method to use?
There was a problem hiding this comment.
https://grpc.io/docs/guides/health-checking/
The health check service on a gRPC server supports two modes of operation:
- Unary calls to the Check rpc endpoint
- Useful for centralized monitoring or load balancing solutions, but does not scale to support a fleet of gRPC client constantly making health checks
- Streaming health updates by using the Watch rpc endpoint
- Used by the client side health check feature in gRPC clients
Watch seems to be the preferred mode for standard gRPC clients.
ikhoon
left a comment
ikhoon
left a comment
There was a problem hiding this comment.
I would be satisfied as long as check runs periodically and watch is implemented.
| } | ||
|
|
||
| try (ClientRequestContextCaptor reqCtxCaptor = Clients.newContextCaptor()) { | ||
| stub.check(builder.build(), new StreamObserver<HealthCheckResponse>() { |
There was a problem hiding this comment.
Check is a unary call so it can't continuously update the upstream's status.
For Check method, we need a scheduler to periodically send Check requests.
| } | ||
|
|
||
| public void start() { | ||
| check(); |
There was a problem hiding this comment.
https://grpc.io/docs/guides/health-checking/
The health check service on a gRPC server supports two modes of operation:
- Unary calls to the Check rpc endpoint
- Useful for centralized monitoring or load balancing solutions, but does not scale to support a fleet of gRPC client constantly making health checks
- Streaming health updates by using the Watch rpc endpoint
- Used by the client side health check feature in gRPC clients
Watch seems to be the preferred mode for standard gRPC clients.
|
Hi, I have started working through these changes... |
|
Hi, I've implemented the requested changes:
Unit tests and checkstyle pass. For the tests, an exception gets logged during the tear down when the health check tries to fire during tear down. I'm not really too sure what to do with that. And the |
|
Tick the box to add this pull request to the merge queue (same as
|
5 participants
Motivation:
Add
GrpcHealthCheckedEndpointGroupBuilderwhich builds a health checked endpoint group whose health comes from a standard gRPC health check service result.Modifications:
GrpcHealthCheckedEndpointGroupBuilderwhich extendsAbstractHealthCheckedEndpointGroupBuilderand creates a new health check functionGrpcHealthCheckerwhich is the health check function that creates and uses a gRPCHealthGrpcstub to check the gRPC health service on the endpoint. If the health check response isSERVING, it is healthy. It is unhealthy if the response is notSERVINGor if there was a request failure.Result: