-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
examples: add example to show how to use the health service #3381
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.
Sorry for the long radio silence. This looks great, thank you! I have a few minor wording changes suggested. In terms of the example, it would be nice to demonstrate the client auto-checking in action, but at the same time I recognize that it's hard to set up, and even then it might be hard for a person to fully grasp what it's doing. An idea I had:
- Server binary starts two servers on two ports exposing the same service. The servers respond with a unique message so you can see which is being used.
- Client is created with a manual resolver that injects both server addresses (typically DNS would do this instead), uses a service config that sets a round_robin LB policy with health checks enabled, and then performs RPCs.
- Client then sends a simple RPC and shows the two servers being alternated. Then one server disables itself and we observe only one server being used on the client.
If that sounds like something you're interested in doing, or if you have a better idea for showing off the feature fully, then it can be done in another PR. If not, I'll add it into the issue and we can leave it open. Either way we can just do the few minor changes in this PR and merge it.
Thanks!
examples/features/health/README.md
Outdated
@@ -0,0 +1,59 @@ | |||
# Health | |||
|
|||
gRPC provides a health library to communicate a systems health to their clients. |
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.
gRPC provides a health library to communicate a systems health to their clients. | |
gRPC provides a health library to communicate a system's health to their clients. |
examples/features/health/README.md
Outdated
gRPC provides a health library to communicate a systems health to their clients. | ||
It works by providing a service definition via the [health/v1](https://github.com/grpc/grpc-proto/blob/master/grpc/health/v1/health.proto) api. | ||
|
||
By using the health library, clients can gracefully fail out servers as they encounter issues. |
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.
By using the health library, clients can gracefully fail out servers as they encounter issues. | |
By using the health library, clients can gracefully avoid using servers as they encounter issues. |
examples/features/health/README.md
Outdated
They can use `Check()` to probe a servers health or they can use `Watch()` to observe changes. | ||
|
||
In most cases, clients do not need to directly check backend servers. | ||
Instead, they can do this transparently when a `healthCheckConfig` is specified. |
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.
Instead, they can do this transparently when a `healthCheckConfig` is specified. | |
Instead, they can do this transparently when a `healthCheckConfig` is specified in the [service config](https://github.com/grpc/proposal/blob/master/A17-client-side-health-checking.md#service-config-changes). |
I'll make the minor changes, but can also work on updating the example to show both direct and transparent use. |
@dfawley updated based on your feedback and included a transparent example |
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.
Very nice, thank you for making the changes! A few comments below.
Also, please check the travis errors (for now it's missing copyright notices on top of client & server main.go
s making it fail.
"google.golang.org/grpc/health" | ||
healthpb "google.golang.org/grpc/health/grpc_health_v1" | ||
) | ||
|
||
var ( | ||
defaultSleep = time.Second * 5 |
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.
Nit: please remove this variable only used once.
time.Sleep(time.Second) | ||
} | ||
} | ||
|
||
func main() { | ||
flag.Parse() | ||
// Following is an example name resolver implementation. Read the name |
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.
Sorry for not being clearer. This implementation is not necessary; please use the manual resolver instead to inject the server addresses: https://godoc.org/google.golang.org/grpc/resolver/manual
Let me know if its usage isn't clear and I can help (we also use this in many tests if you would like to see examples).
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.
I did notice the manual resolver and tried poking around it for a bit. I will take another look at it and how to use it. I was actually thinking about writing a quick static resolver that would accept a scheme like static:///addr1,addr2,...
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.
To use the manual resolver, I think you can do something like:
r, cleanup := manual.GenerateAndRegisterManualResolver()
defer cleanup()
r.InitialState(resolver.State{Addresses: []resolver.Address{{Addr: ... }, ...})
cc, err := grpc.Dial(r.Scheme()+":///unused")
Let me know if you have any problems. Thanks!
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.
Oh, no I figured that. It was just something I thought about writing before.
r, err := c.UnaryEcho(ctx, &pb.EchoRequest{}) | ||
if err == nil { | ||
fmt.Println("UnaryEcho: ", r.GetMessage()) | ||
} |
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.
It would be nice to print something in case there is an error, for debugging purposes. How about unconditionally printing both r.GetMessage()
and err
?
And, actually, if you use different periods for the two servers (5s and 10s as in the example), then we do expect to see timeouts when they are both in the unhealthy phase for more than 1s.
@dfawley updated based on your feedback. I'll keep an eye on the Travis job. Let me know if there is anything further. |
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.
Thanks for the changes. There's just a few small things left.
@@ -1,3 +1,21 @@ | |||
/* | |||
* | |||
* Copyright 2018 gRPC authors. |
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.
Please update to 2020
func callUnaryEcho(c pb.EchoClient) { | ||
ctx, cancel := context.WithTimeout(context.Background(), time.Second) | ||
defer cancel() | ||
r, err := c.UnaryEcho(ctx, &pb.EchoRequest{}) | ||
if err == nil { | ||
if err != nil { | ||
fmt.Println(fmt.Sprintf("UnaryEcho: _, %v", err)) |
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 can be simplified a bit:
fmt.Printf("UnaryEcho: _, %v\n", err)
or
fmt.Println("UnaryEcho: _, ", err)
@@ -1,3 +1,21 @@ | |||
/* | |||
* | |||
* Copyright 2018 gRPC authors. |
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.
Please update date.
@dfawley done! |
I think what all this verbosity means is that |
Run |
Ran gofmt, looks like it's all green. Thanks! |
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, thank you for the contribution!
Resolves #2770