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

examples: add example to show how to use the health service #3381

Merged
merged 8 commits into from Apr 8, 2020
Merged

examples: add example to show how to use the health service #3381

merged 8 commits into from Apr 8, 2020

Conversation

mjpitz
Copy link
Contributor

@mjpitz mjpitz commented Feb 15, 2020

Resolves #2770

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 15, 2020

CLA Check
The committers are authorized under a signed CLA.

@menghanl menghanl added the Type: Documentation Documentation or examples label Feb 20, 2020
@menghanl menghanl added this to the 1.28 Release milestone Feb 20, 2020
@dfawley dfawley modified the milestones: 1.28 Release, 1.29 Release Mar 5, 2020
Copy link
Member

@dfawley dfawley left a 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!

@@ -0,0 +1,59 @@
# Health

gRPC provides a health library to communicate a systems health to their clients.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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).

@dfawley dfawley assigned mjpitz and unassigned dfawley Mar 20, 2020
@mjpitz
Copy link
Contributor Author

mjpitz commented Mar 21, 2020

I'll make the minor changes, but can also work on updating the example to show both direct and transparent use.

@mjpitz
Copy link
Contributor Author

mjpitz commented Mar 21, 2020

@dfawley updated based on your feedback and included a transparent example

Copy link
Member

@dfawley dfawley left a 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.gos making it fail.

"google.golang.org/grpc/health"
healthpb "google.golang.org/grpc/health/grpc_health_v1"
)

var (
defaultSleep = time.Second * 5
Copy link
Member

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
Copy link
Member

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).

Copy link
Contributor Author

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,...

Copy link
Member

Choose a reason for hiding this comment

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

@mjpitz,

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!

Copy link
Contributor Author

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.

Comment on lines 33 to 36
r, err := c.UnaryEcho(ctx, &pb.EchoRequest{})
if err == nil {
fmt.Println("UnaryEcho: ", r.GetMessage())
}
Copy link
Member

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.

@mjpitz
Copy link
Contributor Author

mjpitz commented Apr 3, 2020

@dfawley updated based on your feedback. I'll keep an eye on the Travis job. Let me know if there is anything further.

Copy link
Member

@dfawley dfawley left a 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.
Copy link
Member

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))
Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

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

Please update date.

@mjpitz
Copy link
Contributor Author

mjpitz commented Apr 3, 2020

@dfawley done!

@dfawley
Copy link
Member

dfawley commented Apr 3, 2020

+gofmt -s -d -l .
+tee /dev/stderr
+not read
+read
examples/features/health/client/main.go

I think what all this verbosity means is that client/main.go needs to be run through gofmt. Please check this out, thanks!

@menghanl
Copy link
Contributor

menghanl commented Apr 3, 2020

Run gofmt -s -d -l -w . (adding -w), the code should be updated.

@mjpitz
Copy link
Contributor Author

mjpitz commented Apr 3, 2020

Ran gofmt, looks like it's all green. Thanks!

Copy link
Member

@dfawley dfawley left a 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!

@dfawley dfawley changed the title gh-2770: add documentation around use of the health API examples: add example to show how to use the health service Apr 8, 2020
@dfawley dfawley merged commit 3038e58 into grpc:master Apr 8, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Documentation Documentation or examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better documentation and examples around gRPC health checks
3 participants