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(transport/grpc): grpc client supports health check option #3067

Conversation

youzhixiaomutou
Copy link
Contributor

Description (what this PR does / why we need it):

Refer to #2736 (comment) grpc client healthCheck is force to enable.

But in https://github.com/grpc/grpc/blob/master/doc/health-checking.md#grpc-health-checking-protocol, the Watch() returns a stream resp:

rpc Watch(HealthCheckRequest) returns (stream HealthCheckResponse);

In some special business usecase,I must use nginx or any other gateway to proxy the grpc request. But it is not support stream resp and it will always return write_timeout. It will take some error in my case like:

rpc error: code = Unavailable desc = last connection error: connection active but received health check RPC error: rpc error: code = Internal desc = stream terminated by RST_STREAM with error code: INTERNAL_ERROR"

The nginx plus has impl the health check with https://docs.nginx.com/nginx/admin-guide/load-balancer/grpc-health-check/.
I think it should support an option for user to choice if I want to use the client's health check. And with the #2736 (comment) user should notice it may affects the forward compatibility.

In this pr, it adds a function WithHealthCheck() to disable health check(default is true).

Which issue(s) this PR fixes (resolves / be part of):

Other special notes for the reviewers:

@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2023

Codecov Report

Merging #3067 (1a1b6b2) into main (6cdd818) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 1a1b6b2 differs from pull request most recent head e5bdfaa. Consider uploading reports for the commit e5bdfaa to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main    #3067      +/-   ##
==========================================
+ Coverage   84.34%   84.36%   +0.02%     
==========================================
  Files          88       88              
  Lines        3986     3992       +6     
==========================================
+ Hits         3362     3368       +6     
  Misses        448      448              
  Partials      176      176              
Files Coverage Δ
transport/grpc/client.go 84.42% <100.00%> (+0.80%) ⬆️

Copy link
Member

@shenqidebaozi shenqidebaozi left a comment

Choose a reason for hiding this comment

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

LGTM

@daemon365 daemon365 merged commit 3fc8fb7 into go-kratos:main Nov 2, 2023
34 checks passed
@youzhixiaomutou youzhixiaomutou deleted the feat/transport/grpc_client_supports_health_check_option branch August 23, 2024 09:22
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.

4 participants