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

chore: expose NewClient method to end users #7010

Merged
merged 17 commits into from
Mar 7, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 19 additions & 6 deletions clientconn.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,11 @@
}, nil
}

// newClient returns a new client in idle mode.
func newClient(target string, opts ...DialOption) (conn *ClientConn, err error) {
func newClient(target, defaultScheme string, opts ...DialOption) (conn *ClientConn, err error) {
cc := &ClientConn{
target: target,
conns: make(map[*addrConn]struct{}),
dopts: defaultDialOptions(),
dopts: defaultDialOptions(defaultScheme),
czData: new(channelzData),
}

Expand Down Expand Up @@ -191,6 +190,11 @@
return cc, nil
}

// NewClient returns a new client in idle mode.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 NewClient(target string, opts ...DialOption) (conn *ClientConn, err error) {
return newClient(target, "dns", opts...)

Check warning on line 195 in clientconn.go

View check run for this annotation

Codecov / codecov/patch

clientconn.go#L194-L195

Added lines #L194 - L195 were not covered by tests
}

// DialContext creates a client connection to the given target. By default, it's
// a non-blocking dial (the function won't wait for connections to be
// established, and connecting happens in the background). To make it a blocking
Expand All @@ -208,7 +212,10 @@
// https://github.com/grpc/grpc/blob/master/doc/naming.md.
// e.g. to use dns resolver, a "dns:///" prefix should be applied to the target.
func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *ClientConn, err error) {
cc, err := newClient(target, opts...)
// At the end of this method, we kick the channel out of idle, rather than waiting for the first rpc.
// The eagerConnect option is used to tell the ClientChannel that DialContext was called to make
// this channel, and hence tries to connect immediately.
bruuuuuuuce marked this conversation as resolved.
Show resolved Hide resolved
cc, err := newClient(target, "passthrough", opts...)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -705,6 +712,7 @@
internal.ExitIdleModeForTesting = func(cc *ClientConn) error {
return cc.idlenessMgr.ExitIdleMode()
}
internal.UserSetDefaultScheme = resolver.GetDefaultScheme() != "dns"
bruuuuuuuce marked this conversation as resolved.
Show resolved Hide resolved
}

func (cc *ClientConn) maybeApplyDefaultServiceConfig(addrs []resolver.Address) {
Expand Down Expand Up @@ -1740,8 +1748,13 @@
// We are here because the user's dial target did not contain a scheme or
// specified an unregistered scheme. We should fallback to the default
// scheme, except when a custom dialer is specified in which case, we should
// always use passthrough scheme.
defScheme := resolver.GetDefaultScheme()
// always use passthrough scheme. For either case, we need to respect any overridden
// global defaults set by the user.
defScheme := cc.dopts.defScheme
if defScheme == "" || internal.UserSetDefaultScheme {
bruuuuuuuce marked this conversation as resolved.
Show resolved Hide resolved
defScheme = resolver.GetDefaultScheme()
}

Check warning on line 1756 in clientconn.go

View check run for this annotation

Codecov / codecov/patch

clientconn.go#L1755-L1756

Added lines #L1755 - L1756 were not covered by tests

channelz.Infof(logger, cc.channelzID, "fallback to scheme %q", defScheme)
canonicalTarget := defScheme + ":///" + cc.target

Expand Down
4 changes: 2 additions & 2 deletions clientconn_parsed_target_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import (
)

func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
defScheme := resolver.GetDefaultScheme()
defScheme := "passthrough"
bruuuuuuuce marked this conversation as resolved.
Show resolved Hide resolved
tests := []struct {
target string
wantParsed resolver.Target
Expand Down Expand Up @@ -93,7 +93,7 @@ func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) {
}

func (s) TestParsedTarget_WithCustomDialer(t *testing.T) {
defScheme := resolver.GetDefaultScheme()
defScheme := "passthrough"
tests := []struct {
target string
wantParsed resolver.Target
Expand Down
4 changes: 3 additions & 1 deletion dialoptions.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ type dialOptions struct {
resolvers []resolver.Builder
idleTimeout time.Duration
recvBufferPool SharedBufferPool
defScheme string
}

// DialOption configures how we set up the connection.
Expand Down Expand Up @@ -631,7 +632,7 @@ func withHealthCheckFunc(f internal.HealthChecker) DialOption {
})
}

func defaultDialOptions() dialOptions {
func defaultDialOptions(defScheme string) dialOptions {
return dialOptions{
copts: transport.ConnectOptions{
ReadBufferSize: defaultReadBufSize,
Expand All @@ -643,6 +644,7 @@ func defaultDialOptions() dialOptions {
healthCheckFunc: internal.HealthCheckFunc,
idleTimeout: 30 * time.Minute,
recvBufferPool: nopBufferPool{},
defScheme: defScheme,
}
}

Expand Down
3 changes: 3 additions & 0 deletions internal/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,9 @@ var (

// FromOutgoingContextRaw returns the un-merged, intermediary contents of metadata.rawMD.
FromOutgoingContextRaw any // func(context.Context) (metadata.MD, [][]string, bool)

// UserSetDefaultScheme is set to true if the user has overridden the default resolver scheme.
UserSetDefaultScheme bool
)

// HealthChecker defines the signature of the client-side LB channel health checking function.
Expand Down
7 changes: 4 additions & 3 deletions resolver/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ var (
// m is a map from scheme to resolver builder.
m = make(map[string]Builder)
// defaultScheme is the default scheme to use.
defaultScheme = "passthrough"
defaultScheme = "dns"
bruuuuuuuce marked this conversation as resolved.
Show resolved Hide resolved
)

// TODO(bar) install dns resolver in init(){}.
Expand All @@ -63,7 +63,7 @@ func Get(scheme string) Builder {
}

// SetDefaultScheme sets the default scheme that will be used. The default
// default scheme is "passthrough".
// scheme is "dns".
//
// NOTE: this function must only be called during initialization time (i.e. in
// an init() function), and is not thread-safe. The scheme set last overrides
Expand All @@ -72,7 +72,8 @@ func SetDefaultScheme(scheme string) {
defaultScheme = scheme
bruuuuuuuce marked this conversation as resolved.
Show resolved Hide resolved
}

// GetDefaultScheme gets the default scheme that will be used.
// GetDefaultScheme gets the default scheme that will be used, if no scheme was
// overridden, defaults to the "dns" scheme.
bruuuuuuuce marked this conversation as resolved.
Show resolved Hide resolved
func GetDefaultScheme() string {
return defaultScheme
}
Expand Down