Add new Resolver and Balancer APIs (gRFC L9) #1408

menghanl · 2017-07-31T22:46:18Z

This is the first step for #1388.
See gRFC L9 for details: grpc/proposal#30

This PR adds package balancer and resolver.
It changes ClientConn internals to new APIs and adds a wrapper for v1 balancer.

It does minimal change to ClientConn's behavior:

Blocking dial blocks until the ClientConn's state is READY

$new-bar-2$

dfawley · 2017-08-07T21:19:19Z

balancer/balancer.go

+
+var (
+	// m is a map from name to balancer builder.
+	m map[string]Builder


m = make(map[string]Builder) to save the line in init()

dfawley · 2017-08-07T21:19:57Z

balancer/balancer.go

+// Get returns the resolver builder registered with the given name.
+// If no builder is register with the name, the default pickfirst will
+// be used.
+func Get(name string) (b Builder, ok bool) {


is "ok" necessary? Or can the user check b == nil?

or even:

var defaultBuilder Builder = &pickfirst.Builder{}

-- now "!ok" is not possible (once implemented).

Done.
Return defaultBuilder if map lookup is not ok.

dfawley · 2017-08-07T21:22:36Z

balancer/balancer.go

+// SubConnection represents a gRPC sub connection.
+// Each sub connection contains a list of addresses. gRPC will
+// try to connect to them (in sequence), and stop trying the
+// remainings if one connection was successful.


"// remainder once one connection is successful"

dfawley · 2017-08-07T21:23:02Z

balancer/balancer.go

+// The reconnect backoff will be applied on the list, not a single address.
+// For example, try_on_all_addresses -> backoff -> try_on_all_addresses.
+//
+// All SubConnection starts in IDLE, and will not try to connect. To trigger


"All SubConnections start"

dfawley · 2017-08-07T21:23:35Z

balancer/balancer.go

+// All SubConnection starts in IDLE, and will not try to connect. To trigger
+// the connecting, Balancers must call Connect.
+// When the connection encounters an error, it will reconnect immediately.
+// When the connection becomes IDLE, it will not reconnect unless Connect is


Should we document when this could happen (that a connection goes to IDLE)?

This should be documented in the connectivity semantics doc. Also, it's not clear when a SubConn should go IDLE now...

dfawley · 2017-08-07T23:13:00Z

resolver/resolver.go

+
+// Package resolver defines APIs for name resolution in gRPC.
+// All APIs in this package are experimental.
+package resolver


Many of the comments in the balancer package apply here as well.

dfawley · 2017-08-07T23:25:20Z

resolver/resolver.go

+
+// SetDefaultScheme sets the default scheme that will be used.
+// The default default scheme is "dns".
+func SetDefaultScheme(scheme string) {


Re: balancer comments, how about setting the default scheme to a Builder instead of a string?

The resolver is a bit different from balancer I think.
For balancer, users cannot override the default balancer. The default will always be pickfirst.
For resolver, the default is whatever the users set here.

If we read the map in SetDefaultScheme function, we need to make sure the builder was already registered.

We could make this SetDefaultBuilder(b Builder).
Then if users set the default resolver to one with scheme "blah", and then dial with "blah:///", we should find the correct resolver to use. So we need to register the default Builder as well.

dfawley · 2017-08-07T23:31:05Z

resolver/resolver.go

+	// of resolved addresses.
+	// The address list should be the complete list of new addresses.
+	// Two consecutive call with the same addresses will be noop. It's preferred
+	// that resolver doesn't call this consecutively with non-updated addresses.


I wonder if we even need this sentence. Like, if the resolver refreshes every 30 minutes and calls this, it's not a problem. The idea was to avoid a busy-loop situation, right? I'm not sure who would implement a resolver to continually call NewAddress, or what line of thinking might even lead to that.

dfawley · 2017-08-07T23:32:22Z

resolver/resolver.go

+// Resolver watches for the updates on the specified target.
+// Updates include address updates and service config updates.
+type Resolver interface {
+	// ResolveNow will be called by gRPC to try to resolve the target name again.


For further discussion: can we avoid the need for this by having grpc start up a new Resolver for the same target?

Starting up a new resolver means we need to support switching resolver.
This sounds more complicated than just calling a function on the existing resolver...

dfawley · 2017-08-07T23:34:52Z

clientconn.go

-}
-
-// ConnectivityState indicates the state of a client connection.
-type ConnectivityState int


These changes will break anyone using the connectivity state API (it's new, but, still).

Maybe we can push a PR with only that change in it ASAP while we iterate on this PR?

Done in #1430

Add and use connectivity package for states (grpc#1430) * Add and use connectivity package * Mark cc state APIs as experimental Add balancer, resolver and connectivity package. add balancer_v1_wrapper.go and remove grpclb.go all test passed, how? end2end passed, nil pointer failure, ac.wait return nil transport fix ac.wait return nil transport, races not fixed (accessing cc.balancer) end2end passed, TODO grpclb all test passed add grpclb back move building balancer out from a goroutine to avoid race Otherwise, Dial() and Close() may race. Mark new APIs experimental split resetAddrConn into newAddrConn and connect add acBalancerWrapper rename file to balancer_conn_wrappers.go remove grpclog.Print make TODO(bar) remove Print comments fixes add missing license fix build failure after merge fix race in grpclb

…o SubConn

Implement UpdateAddresses to fix TestPickFirstOrderAllServerUp.

dfawley · 2017-08-23T20:23:00Z

clientconn.go

 	dopts   dialOptions
 	events  trace.EventLog
+	acbw    *acBalancerWrapper


Should this be a SubConn type instead? We only need it so we can notify the balancer that the ac's state changed, IIUC.

dfawley · 2017-08-23T20:32:22Z

balancer_conn_wrappers.go

+	if !ok {
+		return
+	}
+	ccb.cc.removeAddrConn(acbw.getAddrConn(), errConnClosing)


Should this be errConnDrain?

dfawley · 2017-08-23T20:36:17Z

balancer/balancer.go

+	// If it's in the list, the connection will be kept.
+	// If it's not in the list, the connection will gracefully closed, and
+	// a new connection will be created.
+	UpdateAddresses([]resolver.Address)


What SubConn state changes could result from this call if the current address is not in the new list? It should depend on the current state of the subconn, presumably:

Idle->Idle (Would this trigger a callback?)
Connecting->Connecting (to new address) ? (Would this trigger a callback?)
Ready->TransientFailure->Connecting->Ready ?
TransientFailure->Connecting->Ready
Shutdown->Shutdown

For Idle and Shutdown, it won't trigger a callback.
For the other 3 cases, yes, they can all happen.
We actually can have Ready->Connecting->Ready.

I updated the comment to mention this can trigger state update for the subconn.
I also added a TODO to check all the state transitions.

dfawley · 2017-08-23T20:41:57Z

balancer/balancer.go

+	// The returned put() function will be called once the rpc has finished, with the
+	// final status of that RPC.
+	// put may be nil if balancer doesn't care about the RPC status.
+	Pick(ctx context.Context, opts PickOptions) (conn SubConn, put func(PutInfo), err error)


s/put/done/g?

Or maybe rpcDone or rpcEnd?

(I would rather this not seem like the opposite operation as Pick().)

gyuho · 2017-08-28T22:38:58Z

balancer_v1_wrapper.go

+				if oldSC != nil {
+					// Teardown old sc.
+					delete(bw.conns, oldA)
+					delete(bw.connSt, oldSC)


Q. Why is this not protected via bw.mu as in line 187-188?

Good catch! I believe this needs to be protected by the mutex. Will fix it soon.

Fixed. Please take a look.
Thanks for pointing this out!

Np. Thanks!

menghanl changed the title ~~Add new Resolver and Balancer APIs~~ Add new Resolver and Balancer APIs (gRFC L9) Jul 31, 2017

dfawley self-requested a review August 4, 2017 20:50

dfawley self-assigned this Aug 4, 2017

menghanl force-pushed the bar_api_change branch 6 times, most recently from ab7246e to 6aaa555 Compare August 7, 2017 23:34

dfawley reviewed Aug 7, 2017

View reviewed changes

menghanl force-pushed the bar_api_change branch 6 times, most recently from 8997c7e to d3ded16 Compare August 10, 2017 19:12

menghanl added 2 commits August 10, 2017 12:16

Update comments for balancer and resolver, and rename SubConnection t…

b4e43a0

…o SubConn

menghanl force-pushed the bar_api_change branch from d3ded16 to b4e43a0 Compare August 10, 2017 19:16

menghanl added 5 commits August 11, 2017 11:47

rename removeSubConn to removeAddrConn

1678d8a

Add ErrTransientFailure

518b419

Merge branch 'master' into bar_api_change

1374521

Implement UpdateAddresses to fix TestPickFirstOrderAllServerUp.

implement sc.UpdateAddresses

94fab5a

fix datarace in UpdateAddresses

8d17675

dfawley assigned menghanl and unassigned dfawley Aug 23, 2017

fix datarace 2

5ac3fba

menghanl assigned dfawley and unassigned menghanl Aug 23, 2017

dfawley reviewed Aug 23, 2017

View reviewed changes

menghanl added 2 commits August 23, 2017 16:34

rename stuff

efa34c3

modify updateaddresses comment and add todo for state transitions

eafe27c

dfawley approved these changes Aug 24, 2017

View reviewed changes

dfawley added the Type: API Change Breaking API changes (experimental APIs only!) label Aug 24, 2017

gyuho reviewed Aug 28, 2017

View reviewed changes

menghanl added 2 commits August 28, 2017 16:04

protect pickfirst delete conns in v1 balancer wrapper

f0b2bd4

Merge branch 'master' into bar_api_change

1f516b1

menghanl force-pushed the bar_api_change branch from b766426 to 1f516b1 Compare August 29, 2017 20:59

menghanl added 2 commits August 29, 2017 16:37

add error to builder build

12067ee

Merge branch 'master' into bar_api_change

bed9124

menghanl added this to the 1.7 Release milestone Aug 30, 2017

menghanl merged commit 8233e12 into grpc:master Aug 31, 2017

menghanl deleted the bar_api_change branch August 31, 2017 17:59

menghanl mentioned this pull request Nov 29, 2017

New balancer and resolver APIs #1388

Closed

8 tasks

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new Resolver and Balancer APIs (gRFC L9) #1408

Add new Resolver and Balancer APIs (gRFC L9) #1408

menghanl commented Jul 31, 2017 •

edited

Loading

dfawley Aug 7, 2017

menghanl Aug 8, 2017

dfawley Aug 7, 2017

menghanl Aug 8, 2017

dfawley Aug 7, 2017

menghanl Aug 8, 2017

dfawley Aug 7, 2017

menghanl Aug 8, 2017

dfawley Aug 7, 2017

menghanl Aug 8, 2017

dfawley Aug 7, 2017

menghanl Aug 8, 2017 •

edited

Loading

dfawley Aug 7, 2017

menghanl Aug 8, 2017

dfawley Aug 7, 2017

dfawley Aug 7, 2017

menghanl Aug 8, 2017

dfawley Aug 7, 2017

menghanl Aug 8, 2017

dfawley Aug 23, 2017

menghanl Aug 23, 2017

dfawley Aug 23, 2017

menghanl Aug 23, 2017

dfawley Aug 23, 2017

menghanl Aug 24, 2017

dfawley Aug 23, 2017

menghanl Aug 23, 2017

gyuho Aug 28, 2017

menghanl Aug 28, 2017

menghanl Aug 28, 2017

gyuho Aug 29, 2017

Add new Resolver and Balancer APIs (gRFC L9) #1408

Add new Resolver and Balancer APIs (gRFC L9) #1408

Conversation

menghanl commented Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl commented Jul 31, 2017 •

edited

Loading

menghanl Aug 8, 2017 •

edited

Loading