balancer: move Balancer and Picker to V2; delete legacy API

[dfawley]

Updates #3180

---

This change is 

[menghanl]

Reviewed 32 of 33 files at r1.
Reviewable status: 32 of 33 files reviewed, 3 unresolved discussions (waiting on @dfawley and @menghanl)

---

balancer_test.go, line 152 at r1 (raw file):

}

func (s) TestEmptyAddrs(t *testing.T) {

All tests in this file are for the old implementation. We should have equivalent new tests.

This empty address one should cause an error for the new roundrobin, right? I don't remember if that's covered. (No matter what, we can remove this test).

---

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

	picker     balancer.Picker

	// The latest connection error.  TODO: remove when V1 picker is deprecated;

Do we keep this?

---

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

// PickerBuilder creates balancer.Picker.
type PickerBuilder interface {

This will be a breaking change. Should we type alias V2PickerBuilder, too?

[dfawley]

Reviewable status: 32 of 33 files reviewed, 3 unresolved discussions (waiting on @menghanl)

---

balancer_test.go, line 152 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

All tests in this file are for the old implementation. We should have equivalent new tests.

This empty address one should cause an error for the new roundrobin, right? I don't remember if that's covered. (No matter what, we can remove this test).

I'll take a look at all the tests that were deleted one-by-one.

We did not make round_robin error on zero addresses, as it was not clear that such a behavior change was worth making.

---

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

Previously, menghanl (Menghan Li) wrote…

Do we keep this?

Done. (Whoops)

---

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

Previously, menghanl (Menghan Li) wrote…

This will be a breaking change. Should we type alias V2PickerBuilder, too?

Done. Added a couple other V2 symbols, too.

[menghanl]

Reviewed 7 of 7 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dfawley and @menghanl)

---

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

//
// Deprecated: use PickerBuilder instead.
type PickerBuilderV2 = PickerBuilder

V2PickerBuilder

---

internal/balancer/stub/stub.go, line 30 at r2 (raw file):

	// Build is called after ClientConn and BuildOptions are set in
	// BalancerData.  It may be used to initialize BalancerData.Data.
	Build func(*BalancerData)

Built?
AfterBuild?

---

internal/balancer/stub/stub.go, line 93 at r2 (raw file):

// functions.  The name used should be unique.
func Register(name string, bf BalancerFuncs) {
	balancer.Register(bb{name, bf})

Nit: name the fields?

[dfawley]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @menghanl)

---

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

Previously, menghanl (Menghan Li) wrote…

V2PickerBuilder

Done.

---

internal/balancer/stub/stub.go, line 30 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Built?
AfterBuild?

I named it this because it's called during Build, but it is true it's not a stub Build function. How about Init?

---

internal/balancer/stub/stub.go, line 93 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Nit: name the fields?

Done

[menghanl]

Reviewed 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[menghanl]

Reviewed 7 of 7 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dfawley)

---

balancer/balancer.go, line 265 at r4 (raw file):

// DropRPCError wraps err in an error implementing DropRPC() bool, returning
// true.

Also comment about the unknown code in the error?

---

test/balancer_test.go, line 506 at r4 (raw file):

		t.Fatalf("round robin client did not fail after 5 seconds")
	}

Write something about because rr failed, so we wait shorter for pf.

[dfawley]

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

---

balancer/balancer.go, line 265 at r4 (raw file):

Previously, menghanl (Menghan Li) wrote…

Also comment about the unknown code in the error?

Done. Also simplified the code here and in picker_wrapper.go that converts.

---

test/balancer_test.go, line 506 at r4 (raw file):

Previously, menghanl (Menghan Li) wrote…

Write something about because rr failed, so we wait shorter for pf.

Done.

[menghanl]

Reviewed 2 of 3 files at r5, 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved

[jqll]

This change breaks google-api-go-client, see googleapis/google-api-go-client#441. Not sure if you want to roll back until google-api-go-client fixes the issue?

[dfawley]

Yes, this is a breaking change.

Notice was given in the form of deprecation warnings for 6 months and #3180 has been pinned on our repo for two months. You can vendor or use go module replace directives to pin grpc to an older version if a short-term workaround is required.

[pboothe]

This change breaks all other Google Cloud libraries that transitively depend on this. I didn't know I was using this library, but because I am using an official Google Cloud library that happens to use a different official Google Cloud library that happens to use this library, my team's code was broken by this change. This change has broken other Google Cloud libraries (most specifically the API library, which is used approximately everywhere), which then have passed the breakage on to me as a Monday-morning surprise.

It doesn't just break GRPC users who didn't update, it also broke google.golang.org/api/option/option.go, which can no longer compile. Which means that all Go code that uses options (or transitively uses options) when using the Google cloud API doesn't compile. This change has broken every Google Cloud library build, because it broke that one file. I understand not wanting to roll back, but by breaking that one file, this change breaks a lot more than you think.

To be specific, because of this change, all users of:

• Google Cloud Storage
• Bigquery
• Cloud Datastore
• Cloud Bigtable
• Cloud Pubsub

can no longer build Go clients, because the Go libraries for those services all depends on that broken file. This change has had a much wider breakage radius than you realize.