-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
balancer: move Balancer and Picker to V2; delete legacy API #3301
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r5, 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved
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? |
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 |
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 To be specific, because of this change, all users of:
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. |
Updates #3180
This change is