grpclb, dns: pass balancer addresses via resolver.State #3614
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change implements gRFC A26: gRPCLB Selection: https://github.com/grpc/proposal/blob/master/A26-grpclb-selection.md Note that addresses passed to gRPCLB through resolver.State.Attributes will not automatically select the "grpclb" load balancing policy. This will result in a behavior change for users currently using the DNS name resolver and relying upon that behavior. Instead, a service config specifying grpclb must be used. For example: { "loadBalancingConfig": [ {"grpclb": {}} ] } While this is a behavior change for gRPC-Go, it brings gRPC in line with the intended implementation for gRPCLB support, and the behavior of the C and Java gRPC implementations, and is considered a bug fix. For now, and until support for the resolver.GRPCLB value is deleted, the "grpclb" load balancing policy will still be selected automatically when addresses with those types are produced by a name resolver. This will prevent custom resolvers from being impacted by this behavior change until they need to be modified due to the deletion of this deprecated value.
menghanl
reviewed
May 12, 2020
| // stateDataAttributesKey is the key to use for storing StateData in Attributes. | ||
| type sdKeyType string | ||
|
|
||
| const sdKey = sdKeyType("grpc.grpclb.state_data") |
There was a problem hiding this comment.
Bikeshedding: grpc.grpclb.state?
And package/struct name to state?
There was a problem hiding this comment.
Done. Maybe there are too many "states" now, though?
There was a problem hiding this comment.
What other states? Those are types, not variables?
There was a problem hiding this comment.
resolver.State, balancer.State, now a state package. With import renaming this one isn't really a problem in practice, but it could be confusing to have so many things called "state".
| newAddrs = append(newAddrs, resolver.Address{Addr: addr, Type: resolver.GRPCLB, ServerName: s.Target}) | ||
| // Use Type: Backend instead of GRPCLB since we will be returning | ||
| // these addresses through attributes instead of Addresses. | ||
| newAddrs = append(newAddrs, resolver.Address{Addr: addr, Type: resolver.Backend, ServerName: s.Target}) |
There was a problem hiding this comment.
Don't set Type? Is Backend the default?
|
|
||
| state := &resolver.State{Addresses: addrs} | ||
| if len(srv) > 0 { | ||
| *state = statedata.Set(*state, &statedata.StateData{BalancerAddresses: srv}) |
There was a problem hiding this comment.
How many copies of state does this line make?
Is it better to change the line above to not take the pointer?
| generateSC("foo.bar.com"), | ||
| }, | ||
| { | ||
| "srv.ipv4.single.fake", | ||
| []resolver.Address{{Addr: "2.4.6.8" + colonDefaultPort}, {Addr: "1.2.3.4:1234", Type: resolver.GRPCLB, ServerName: "ipv4.single.fake"}}, | ||
| []resolver.Address{{Addr: "2.4.6.8" + colonDefaultPort}}, | ||
| []resolver.Address{{Addr: "1.2.3.4:1234", Type: resolver.Backend, ServerName: "ipv4.single.fake"}}, |
There was a problem hiding this comment.
If you change the resolver to not explicitly set Backend, change the the tests, too.
| sd := statedata.Get(state) | ||
| if (sd == nil && len(a.grpclbAddrs) > 0) || | ||
| (sd != nil && !reflect.DeepEqual(a.grpclbAddrs, sd.BalancerAddresses)) { | ||
| t.Errorf("Resolved state of target: %q = %+v (StateData=%+v), want state.Attributes.StateData=%+v\n", a.target, state, sd, a.grpclbAddrs) |
menghanl
approved these changes
May 26, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change implements gRFC A26: gRPCLB Selection:
https://github.com/grpc/proposal/blob/master/A26-grpclb-selection.md
Note that addresses passed to gRPCLB through resolver.State.Attributes will not
automatically select the "grpclb" load balancing policy. This will result in a
behavior change for users currently using the DNS name resolver and relying
upon that behavior. Instead, a service config specifying grpclb must be used.
For example:
While this is a behavior change for gRPC-Go, it brings gRPC in line with the
intended implementation for gRPCLB support, and the behavior of the C and Java
gRPC implementations, and is considered a bug fix for that reason.
For now, and until support for the resolver.GRPCLB value is deleted, the
"grpclb" load balancing policy will still be selected automatically when
addresses with those types are produced by a name resolver. This will prevent
custom resolvers from being impacted by this behavior change until they need to
be modified due to the deletion of this deprecated value.