Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Support gRPC health checking #30

Merged
merged 3 commits into from
Aug 28, 2020
Merged

Conversation

honnix
Copy link
Member

@honnix honnix commented Aug 28, 2020

TL;DR

Support gRPC health checking.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

The implementation of https://github.com/grpc/grpc/blob/master/doc/health-checking.md is part of go gRPC lib.

Tracking Issue

flyteorg/flyte#489

Follow-up issue

NA

This addresses flyteorg/flyte#489.

For now only server level health checking is supported but we could
extend to service level when needed later.
@honnix
Copy link
Member Author

honnix commented Aug 28, 2020

@kumare3 Would like to take a look at this as well? Thanks.


healthServer := health.NewServer()
healthServer.SetServingStatus("", grpc_health_v1.HealthCheckResponse_SERVING)
grpc_health_v1.RegisterHealthServer(grpcServer, healthServer)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 nice
nit: we can rename the http health check method below to serveHTTPHealthcheck, this is for integration into proxies like Envoy's health check

Copy link
Member Author

Choose a reason for hiding this comment

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

PTAL again and help merge it. Thanks.

chanadian
chanadian previously approved these changes Aug 28, 2020
@chanadian
Copy link
Contributor

Looks good to me @honnix , @kumare3 would you like to coordinate merging and getting this out since your a deploying on cadence?

@kumare3 kumare3 requested a review from chanadian August 28, 2020 18:54
@kumare3
Copy link
Contributor

kumare3 commented Aug 28, 2020

LGTM, Merging

@kumare3 kumare3 merged commit 8a13d53 into flyteorg:master Aug 28, 2020
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants