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

feat: add grpc health service #382

Merged
merged 3 commits into from
Sep 7, 2022

Conversation

hongalex
Copy link
Collaborator

@hongalex hongalex commented Sep 7, 2022

What type of PR is this?

/kind feat

What this PR does / Why we need it:
Adds gRPC health check service to the grpc server.

Which issue(s) this PR fixes:
Closes #378

Special notes for your reviewer:
Replaces part of #381

@hongalex hongalex requested review from yuryu and lorangf and removed request for yuryu September 7, 2022 20:30
@yuryu
Copy link
Member

yuryu commented Sep 7, 2022

overall lgtm. Do you think it makes sense to add a simple test like

c := healthgrpc.NewHealthClient(conn)
got, err := c.Check(ctx, request)
if err != nil {
 // error
}
if got is not what we want {
  // error
}

Copy link
Member

@yuryu yuryu left a comment

Choose a reason for hiding this comment

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

lgtm.

This is more like a wishlist rather than a request but I guess other tests should also use Run() and would also want to change Run() to accept a Listener instead of direct network address/port so that this test can use bufconn instead.

Service: serviceName,
})
if err != nil {
t.Fatalf("healthClient.Check err: %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.

t.Fatalf("healthClient.Check err: %v", err)
}
if want := healthgrpc.HealthCheckResponse_SERVING; got.Status != want {
t.Fatalf("hc.Check got: %v, want: %v", got.Status, want)
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@hongalex
Copy link
Collaborator Author

hongalex commented Sep 7, 2022

This is more like a wishlist rather than a request but I guess other tests should also use Run() and would also want to change Run() to accept a Listener instead of direct network address/port so that this test can use bufconn instead.

Agreed. I'll open an issue for this.

@hongalex hongalex merged commit a7415ce into googleforgames:main Sep 7, 2022
@hongalex hongalex deleted the feat-grpc-health branch September 7, 2022 21:31
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.

Support a health check endpoint to use as readiness and liveness kubernetes probes
2 participants