balancer: add V2Picker, ClientConn.UpdateState, SubConnState.ConnectionError

[dfawley]

Also implement V2 versions of base.*, xds, pickfirst, grpclb, and round robin balancers.

---

This change is 

[menghanl]

Reviewed 11 of 34 files at r1.
Reviewable status: 11 of 34 files reviewed, 6 unresolved discussions (waiting on @dfawley and @menghanl)

---

balancer_conn_wrappers.go, line 176 at r1 (raw file):

}

func (ccb *ccBalancerWrapper) UpdateBalancerState(s connectivity.State, p balancer.Picker) {

ccb.UpdateState(balancer.State{s, p})

But this allocates memory. So call UpdateBalancerState in UpdateState?

---

grpc_test.go, line 28 at r1 (raw file):

	"google.golang.org/grpc/internal/leakcheck"

	_ "google.golang.org/grpc/grpclog/glogger"

Did we decide which logger to use?

---

picker_wrapper.go, line 35 at r1 (raw file):

)

type v2PickerWrapper struct {

oldPickerWrapper?
picker2v2picker? 😮

And add some comment. The purpose of this struct is not very clear.

---

picker_wrapper.go, line 37 at r1 (raw file):

type v2PickerWrapper struct {
	picker balancer.Picker
	pw     *pickerWrapper // for accessing the latest connection error

Move connErrMu and connErr to a different struct?

---

picker_wrapper.go, line 84 at r1 (raw file):

// updatePicker is called by UpdateBalancerState. It unblocks all blocked pick.
func (pw *pickerWrapper) updatePicker(p balancer.Picker) {
	pw.mu.Lock()

pw.updatePickerV2(&v2PickerWrapper{picker: p, pw: pw})

---

picker_wrapper.go, line 134 at r1 (raw file):

// - the subConn returned by the current picker is not READY
// When one of these situations happens, pick blocks until the picker gets updated.
func (pw *pickerWrapper) pick(ctx context.Context, failfast bool, info balancer.PickInfo) (transport.ClientTransport, func(balancer.DoneInfo), error) {

Remove context from parameters?

[dfawley]

Reviewable status: 11 of 34 files reviewed, 6 unresolved discussions (waiting on @menghanl)

---

balancer_conn_wrappers.go, line 176 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

ccb.UpdateState(balancer.State{s, p})

But this allocates memory. So call UpdateBalancerState in UpdateState?

Since we're planning to delete UpdateBalancerState, I thought this would be preferable. Also, the memory, if not allocated on the stack, is minimal, for an infrequent operation.

---

grpc_test.go, line 28 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Did we decide which logger to use?

I want to use a logger that uses an underlying testing.T, but the status quo is fine for now. In this PR I've consolidated us down to a single import of glogger, rather than importing it ~3 times.

---

picker_wrapper.go, line 35 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

oldPickerWrapper?
picker2v2picker? 😮

And add some comment. The purpose of this struct is not very clear.

Kept the name since it's short-lived and anything will be confusing here. Added some comments. PTAL

---

picker_wrapper.go, line 37 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Move connErrMu and connErr to a different struct?

Done.

---

picker_wrapper.go, line 84 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

pw.updatePickerV2(&v2PickerWrapper{picker: p, pw: pw})

Done

---

picker_wrapper.go, line 134 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Remove context from parameters?

It's true it is redundant here. However, ctx here is expected to control the operation of the pick function. When we pass Ctx in PickInfo to the picker's Pick function, it is there for informational purposes only, i.e. Pick is not expected to check info.Ctx.Err or block on info.Ctx.Done. I think it's worth keeping it separate for this reason. We could make pick construct the PickInfo if you are concerned about the duplication (?).

[menghanl]

Reviewed 2 of 34 files at r1, 1 of 1 files at r2.
Reviewable status: 13 of 34 files reviewed, 2 unresolved discussions (waiting on @dfawley and @menghanl)

---

balancer/balancer.go, line 122 at r1 (raw file):

// State contains the balancer's state relevant to the gRPC ClientConn.
type State struct {
	// State controls the connectivity state of the ClientConn

Controls? Or contains

---

balancer/base/balancer.go, line 138 at r2 (raw file):

			b.picker = NewErrPicker(balancer.ErrTransientFailure)
		} else {
			b.v2Picker = NewErrPickerV2(balancer.ErrTransientFailure)

Add connection error from SubConnState to the transient failure error

[dfawley]

Reviewable status: 13 of 34 files reviewed, 2 unresolved discussions (waiting on @menghanl)

---

balancer/balancer.go, line 122 at r1 (raw file):

Controls

I think I prefer "controls" - whatever the balancer sets here is what the clientconn will report.

---

balancer/base/balancer.go, line 138 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Add connection error from SubConnState to the transient failure error

Done

[menghanl]

Reviewed 1 of 34 files at r1.
Reviewable status: 14 of 34 files reviewed, 4 unresolved discussions (waiting on @dfawley and @menghanl)

---

balancer/base/base.go, line 49 at r2 (raw file):

	// Build takes a slice of ready SubConns, and returns a picker that will be
	// used by gRPC to pick a SubConn.
	Build(readySCs map[resolver.Address]balancer.SubConn, opts PickerBuildOptions) balancer.V2Picker

map[balancer.SubConn]SubConnInfo

type SubConnInfo struct {
  address resolver.Address
}

---

balancer/base/base.go, line 64 at r2 (raw file):

// NewBalancerBuilderV2 returns a balancer builder. The balancers
// built by this builder will use the picker builder to build pickers.
func NewBalancerBuilderV2(name string, pb V2PickerBuilder) balancer.Builder {

NewBalancerBuilderV2Options, for affinity?

[menghanl]

Reviewed 18 of 34 files at r1, 4 of 4 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dfawley)

---

balancer/base/balancer.go, line 203 at r3 (raw file):

	if (s == connectivity.Ready) != (oldS == connectivity.Ready) ||
		(b.state == connectivity.TransientFailure) != (oldAggrState == connectivity.TransientFailure) {
		fmt.Println("state: ", state)

Remove

---

health/client.go, line 105 at r3 (raw file):

			// Reports unhealthy if server's Watch method gives an error other than UNIMPLEMENTED.
			if err != nil {
				setConnectivityState(connectivity.TransientFailure, errors.New("health check RPC error"))

More details in the error message? At least make it clear that the state is set to transient failure because (gRPC's) health check failed

---

health/client.go, line 114 at r3 (raw file):

				setConnectivityState(connectivity.Ready, nil)
			} else {
				setConnectivityState(connectivity.TransientFailure, errors.New("health check failed"))

Similar here.

[dfawley]

Reviewable status: 31 of 34 files reviewed, 5 unresolved discussions (waiting on @dfawley and @menghanl)

---

balancer/base/base.go, line 49 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

map[balancer.SubConn]SubConnInfo

type SubConnInfo struct {
  address resolver.Address
}

Done.

---

balancer/base/base.go, line 64 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

NewBalancerBuilderV2Options, for affinity?

Done.

---

health/client.go, line 105 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

More details in the error message? At least make it clear that the state is set to transient failure because (gRPC's) health check failed

PTAL

---

health/client.go, line 114 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Similar here.

Similar here.

[dfawley]

Reviewable status: 31 of 34 files reviewed, 5 unresolved discussions (waiting on @menghanl)

---

balancer/base/balancer.go, line 203 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Remove

Done.

[menghanl]

Reviewed 4 of 4 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved