-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Upgrade to google.golang.org/grpc@1.35.0
#12636
Conversation
This is intended so that `go mod tidy` can be used to re-generate imports (to prepare for an update of `google.golang.org/grpc`.
The official recommendation is to move to `google.golang.org/grpc/resolver` but this is not exactly a drop-in replacement.
This is because the `grpc/naming` breaking change happened in `v1.30.0` (i.e. we need to get past `v1.29.1`). See: https://github.com/grpc/grpc-go/releases/tag/v1.30.0
The two implementations updated satisfy `(google.golang.org/grpc/balancer).Picker`, see the history of this interface: - https://pkg.go.dev/google.golang.org/grpc@v1.29.1/balancer#Picker - https://pkg.go.dev/google.golang.org/grpc@v1.30.0/balancer#Picker - https://pkg.go.dev/google.golang.org/grpc@v1.35.0/balancer#Picker
The `baseBalancer` struct satisfies `(google.golang.org/grpc/balancer).Balancer`, see the history of this interface: - https://pkg.go.dev/google.golang.org/grpc@v1.29.1/balancer#Balancer - https://pkg.go.dev/google.golang.org/grpc@v1.30.0/balancer#Balancer - https://pkg.go.dev/google.golang.org/grpc@v1.35.0/balancer#Balancer
See etcd-io#12564 (comment) for how I determined which order to visit packages in.
// UpdateClientConnState implements "grpc/balancer.Balancer" interface. | ||
func (bb *baseBalancer) UpdateClientConnState(ccs balancer.ClientConnState) error { | ||
bb.HandleResolvedAddrs(ccs.ResolverState.Addresses, nil) | ||
// TODO: This **should** incorporate the lines that warn in `HandleResolvedAddrs`. |
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.
E.g. here:
etcd/client/v3/balancer/balancer.go
Line 145 in 5dcd459
bb.lg.Warn("HandleResolvedAddrs called with error", zap.String("balancer-id", bb.id), zap.Error(err)) |
My goal here was to avoid a big diff in .go
source files to make it easier for maintainers to grok what has changed. Notice that HandleResolvedAddrs()
is no longer part of the Balancer
interface:
- https://pkg.go.dev/google.golang.org/grpc@v1.29.1/balancer#Balancer
- https://pkg.go.dev/google.golang.org/grpc@v1.35.0/balancer#Balancer
So we could just rename HandleResolvedAddrs()
to UpdateClientConnState()
and just return err
after some / all of the bb.lg.Warn()
statements.
@@ -193,6 +205,11 @@ func (bb *baseBalancer) HandleResolvedAddrs(addrs []resolver.Address, err error) | |||
} | |||
} | |||
|
|||
// UpdateSubConnState implements "grpc/balancer.Balancer" interface. | |||
func (bb *baseBalancer) UpdateSubConnState(sc balancer.SubConn, s balancer.SubConnState) { | |||
bb.HandleSubConnStateChange(sc, s.ConnectivityState) |
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.
Ditto here on rename / repurpose of old code. HandleSubConnStateChange()
is gone from the Balancer
interface so we could just implement that here.
ConnectivityState: bb.connectivityRecorder.GetCurrentState(), | ||
Picker: bb.picker, | ||
} | ||
bb.currentConn.UpdateState(state) |
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.
The ClientConn
interface is what has changed here:
func (ep *errPicker) Pick(context.Context, balancer.PickInfo) (balancer.SubConn, func(balancer.DoneInfo), error) { | ||
return nil, nil, ep.err | ||
func (ep *errPicker) Pick(balancer.PickInfo) (balancer.PickResult, error) { | ||
return balancer.PickResult{}, ep.err |
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.
@@ -52,12 +51,12 @@ type rrBalanced struct { | |||
func (rb *rrBalanced) String() string { return rb.p.String() } | |||
|
|||
// Pick is called for every client request. | |||
func (rb *rrBalanced) Pick(ctx context.Context, opts balancer.PickInfo) (balancer.SubConn, func(balancer.DoneInfo), error) { | |||
func (rb *rrBalanced) Pick(opts balancer.PickInfo) (balancer.PickResult, error) { |
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.
@@ -0,0 +1,33 @@ | |||
package grpcnaming |
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.
Source reference: https://github.com/grpc/grpc-go/blob/v1.29.1/naming/naming.go
This is to address CI failure ``` FAIL: inconsistent versions for depencency: google.golang.org/genproto - google.golang.org/genproto@v0.0.0-20200526211855-cb27e3aa2013 from: go.etcd.io/etcd/api/v3 - google.golang.org/genproto@v0.0.0-20200806141610-86f49bd18e98 from: go.etcd.io/etcd/server/v3 FAIL: inconsistent dependencies FAIL: 'dep' failed at Wed Jan 20 21:00:57 UTC 2021 ```
I fully support desire to go to grpc-1.35, but we cannot make it without solving the protobuf-1.4.2. dependency problem first.
|
Gotcha, seeing some of these failing tests I was starting to patch together that there was a breaking change in core The core issue is the
Mind shedding more light on this? The changes here preserve the existing behavior, I'm curious what the concern might be. |
…g/protobuf@v1.3.3`. Done via: ``` git grep -l 'google.golang.org/grpc v1.35.0' -- '*go.mod' | xargs sed -i '' s/'google.golang.org\/grpc v1.35.0'/'google.golang.org\/grpc v1.30.0'/g git grep -l 'github.com/golang/protobuf v1.4.2' -- '*go.mod' | xargs sed -i '' s/'github.com\/golang\/protobuf v1.4.2'/'github.com\/golang\/protobuf v1.3.3'/g ```
This included a `replace` directive for the `protobuf` version.
This included a `replace` directive for the `protobuf` version.
This included a `replace` directive for the `grpc` version.
This included a `replace` directive for the `grpc` and `protobuf` versions.
This included a `replace` directive for the `grpc` version.
@ptabor I didn't realize 1.4.x is the reason the It seems the only way to cross the 1.3->1.4 chasm for UPDATE: I now see #12124 (comment), good to know you are on top of things! Sorry for the noise. UPDATE: Sadly https://blog.golang.org/protobuf-apiv2 makes no mention of faster marshalling and unmarshalling without reflection. |
https://github.com/etcd-io/etcd/blob/master/Documentation/dev-guide/grpc_naming.md Describes usage of etcd as grpc name resolver in customer's application. This requires support from grpc layer that for these interfaces for 1.30+ is gone.
Elaborated on this in: #12197 (comment) Proposal to drive this effort to completion:
|
Could you, please, extract the client/v3/balancer/... change alone. I think it should be compatible with: |
@ptabor will do. I'll send it out as a new PR. Thanks. |
Closing in favor of #12658. May revisit these changes at a later date (likely to just cherry-pick from this PR into a new PR). |
This is VERY similar to #12609, but I tried to minimize the diffs by using import aliases and also resolved some breakages in interfaces from
google.golang.org/grpc/balancer
.This does not try to fully migrate to the new
resolver
pattern as in #12614, but just provides a bridge to the latest version ofgoogle.golang.org
.Getting past
1.29.1
(there were several breaking changes in1.30.0
, including the removal ofgoogle.golang.org/grpc/naming
) is critical to be able to useetcd
in codebases that also usek8s>=1.17
withgo mod
.