Replace grpc.DialContext and grpc.WithBlock with grpc.NewClient #6026
Conversation
karmada-bot
added
the
kind/deprecation
karmada-bot
added
the
size/M
|
@zhzhuang-zju please help take a look. Thank you so much! |
|
/assign |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #6026 +/- ##
==========================================
+ Coverage 48.35% 48.36% +0.01%
==========================================
Files 665 666 +1
Lines 54831 54838 +7
==========================================
+ Hits 26512 26521 +9
+ Misses 26599 26597 -2
Partials 1720 1720
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
/retest |
pkg/util/grpcconnection/config.go
Outdated
| cc, err = createGRPCConnection(path, timeout, opts...) | ||
| if err == nil { | ||
| return cc, nil | ||
| cc, err = grpc.NewClient(path, opts...) |
There was a problem hiding this comment.
@seanlaii I found that grpc.NewClient also returns a client when path doesn't exist, with no errors reported. That's not what we expected.
Can we proceed to refine according to the method of validating grpc configuration mentioned in the document?
Some users of Dial use it as a way to validate the configuration of their system. If you wish to maintain this behavior but migrate to NewClient, you can call GetState, then Connect if the state is Idle and WaitForStateChange until the channel is connected. However, if this fails, it does not mean that your configuration was bad - it could also mean the service is not reachable by the client due to connectivity reasons.There was a problem hiding this comment.
Sure! Thanks for the advice!
seanlaii
force-pushed
the
grpc
branch
3 times, most recently
from
9ced984 to
e611c35
Compare
| // grpc.DialContext is deprecated. TODO: Perhaps need to reconsider the approach in a future PR | ||
| //nolint:staticcheck | ||
| cc, err := grpc.DialContext(ctx, path, opts...) | ||
| cc, err := grpc.NewClient(path, opts...) |
There was a problem hiding this comment.
| cc, err := grpc.NewClient(path, opts...) | |
| conn, err = grpc.NewClient(path, opts...) |
We can use the variable defined in return types, which makes the defer function clear about which error it referring to.
There was a problem hiding this comment.
I tried it and encountered an issue: if there is an error, conn will be set to nil, resulting in nil dereference when trying to use conn.Close in defer function. Therefore, I changed it back, which also follows the implementation in here.
Please take a look. Thank you!
There was a problem hiding this comment.
I tried it and encountered an issue: if there is an error, conn will be set to nil, resulting in nil dereference when trying to use conn.Close in defer function.
Nice finding! Seems you tested the abnormal case, thanks.
Signed-off-by: wei-chenglai <[email protected]>
There was a problem hiding this comment.
/lgtm
@chaosi-zju @zhzhuang-zju
Do you have any further comments?
| // grpc.DialContext is deprecated. TODO: Perhaps need to reconsider the approach in a future PR | ||
| //nolint:staticcheck | ||
| cc, err := grpc.DialContext(ctx, path, opts...) | ||
| cc, err := grpc.NewClient(path, opts...) |
There was a problem hiding this comment.
I tried it and encountered an issue: if there is an error, conn will be set to nil, resulting in nil dereference when trying to use conn.Close in defer function.
Nice finding! Seems you tested the abnormal case, thanks.
|
/remove-kind deprecation |
karmada-bot
added
kind/cleanup
/lgtm, thanks @seanlaii |
| //nolint:staticcheck | ||
| grpc.WithBlock(), | ||
| } | ||
| var opts []grpc.DialOption |
There was a problem hiding this comment.
@seanlaii Thanks for your work! Just a suggestion, function name DialWithTimeOut can be considered to be changed to NewClient
There was a problem hiding this comment.
I'm ok with the DialWithTimeOut which clearly explains actions in this function.
There was a problem hiding this comment.
Get it, please ignore this comment
karmada-bot
added
the
approved
|
/lgtm PS: re-trigger prow bot |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind deprecation
What this PR does / why we need it:
grpc.DialContextandgrpc.WithBlockare deprecatedWhich issue(s) this PR fixes:
Part of #5796
Special notes for your reviewer:
Replace
grpc.DialContextandgrpc.WithBlockwithgrpc.NewClientto follow the recommended pattern in https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md.Since we only allow users to configure the authentication setting of the grpc client without any custom dial options, I think we should be safe to perform the replacement.
Does this PR introduce a user-facing change?: