chore: expose NewClient method to end users
#7010
Conversation
This will allow users to create a connection, leaving it in idle state, rather than having it automatically try and connect in the background, which is the current behavior of `Dial`. Leaving the connection in idle state is more consistent with how other grpc implementations deal with connections
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #7010 +/- ##
=======================================
Coverage 82.43% 82.43%
=======================================
Files 296 296
Lines 31463 31471 +8
=======================================
+ Hits 25937 25944 +7
- Misses 4468 4470 +2
+ Partials 1058 1057 -1
|
There was a problem hiding this comment.
One more thing needs to be figured out before we can do this: we want the default name resolver used when calling NewClient directly by users to be dns instead of the awkward and non-standard passthru used by Dial.
However, the default is set globally, and can be overridden, so we need to come up with a way to have a different default for the two different paths, override them both when the user calls resolver.SetDefaultScheme, and have the proper code path use the proper one. (This is mentioned in #1786.)
Let me know if you're interested in taking that work on as well.
Sure I can take a stab at this |
|
@dfawley lmk if this was what you were thinking, also any naming suggestions would be appreciated 😅 |
| @@ -191,6 +190,11 @@ func newClient(target string, opts ...DialOption) (conn *ClientConn, err error) | |||
| return cc, nil | |||
| } | |||
|
|
|||
| // NewClient returns a new client in idle mode. | |||
There was a problem hiding this comment.
Just FYI / for the next reviewer's reference:
After this PR, I plan to rewrite things a little bit. I'd like this to become the primary API for users, and to call Dial and DialContext both "deprecated" in preference of this. As such, this will contain a bit more documentation, and the others will state they call it and then initiate and wait for a connection.
There was a problem hiding this comment.
@dfawley ok now with the newest release both are deprecated. But how do I add a timeout?
When I try the follwing:
conn, err := grpc.NewClient(addr,
grpc.WithTransportCredentials(creds),
grpc.WithBlock(),
grpc.WithChainUnaryInterceptor(interceptors...),
grpc.WithTimeout(dialTimeout),
)the linter tells me that grpc.WithTimeout is deprecated I should use grpc.DialContext (which is deprecated too). So how do I use grpc.NewClient with grpc.WithBlock that doesn't block forever?
There was a problem hiding this comment.
Setting a timeout when creating a client is an anti-pattern, and is not possible in any other gRPC implementations.
See https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md#dialing-in-grpc for more info.
There was a problem hiding this comment.
I agree that connection failures should always be checked, not only on start-up.
But if you're working with setups such as Kubernetes, you don't want your new pod to pass liveness/readiness checks in case it has an invalid connection string. It's way safer to have this "ping" check to confirm there are no misconfigurations in your new deployment, before you terminate your previous pods.
There was a problem hiding this comment.
There are other, better, more robust ways of doing that. For example, health checking: https://github.com/grpc/proposal/blob/master/A17-client-side-health-checking.md
You can also use the connectivity state API if you just want to see if the client was able to connect: https://pkg.go.dev/google.golang.org/grpc#ClientConn.GetState and https://pkg.go.dev/google.golang.org/grpc#ClientConn.WaitForStateChange
| func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) { | ||
| defScheme := resolver.GetDefaultScheme() | ||
| dialScheme := resolver.GetDefaultScheme() |
There was a problem hiding this comment.
I believe this should probably try to re-create the initial state in case another test calls SetDefaultScheme. I.e. do resolver.SetDefaultScheme("passthrough") and internal.UserSetDefaultScheme = false?
resolver/resolver.go
Outdated
| // SetDefaultScheme sets the default scheme that will be used. The default | ||
| // default scheme is "passthrough". | ||
| // scheme is "passthrough". |
There was a problem hiding this comment.
"default default" was not actually a typo. But maybe would be clearer if it said "the default scheme is initially set to ..." or something like that.
This will allow users to create a connection, leaving it in idle state, rather than having it automatically try and connect in the background, which is the current behavior of
Dial. Leaving the connection in idle state is more consistent with how other grpc implementations deal with connectionsLooks like this change was already planned, but just has not been commited yet
grpc-go/clientconn.go
Lines 222 to 225 in 99ded5c
RELEASE NOTES:
grpc.NewClientto allow users to create new clients with connections remaining in idle state