Implementation of A68 random_subsetting LB policy. #8650
Conversation
Signed-off-by: marek-szews <[email protected]>
easwars
added
Type: Feature
|
@marek-szews : Could you please get the tests to pass. Thanks. |
| SubsetSize uint64 `json:"subset_size,omitempty"` | ||
|
|
||
| ChildPolicy *iserviceconfig.BalancerConfig `json:"child_policy,omitempty"` |
There was a problem hiding this comment.
We generally use camelCase for the json annotations since the protobuf library uses that when marshalling protobuf messages to JSON. So, please change these to use camelCase. Thanks.
| return b | ||
| } | ||
|
|
||
| // LBConfig is the config for the outlier detection balancer. |
There was a problem hiding this comment.
This comment needs to be updated.
| } | ||
|
|
||
| // LBConfig is the config for the outlier detection balancer. | ||
| type LBConfig struct { |
There was a problem hiding this comment.
Is there a reason for this type to be exported. I don't see a reason here. So, if there is no reason, please unexport it. The fields can remain exported since you might want that for JSON marshal/unmarshal.
| // Default top layer values. | ||
| SubsetSize: 10, |
There was a problem hiding this comment.
Where is this default value specified?
| } | ||
|
|
||
| // if someonw needs subsetSize == 1, he should use pick_first instead | ||
| if lbCfg.SubsetSize < 2 { |
There was a problem hiding this comment.
The gRFC only says the following:
// The value is required and must be greater than 0.
Is the condition that the size be at least 2 specified somewhere else?
| return addressesSet[i].hash < addressesSet[j].hash | ||
| }) | ||
|
|
||
| b.logger.Infof("resulting subset: %v", addressesSet[:b.cfg.SubsetSize]) |
There was a problem hiding this comment.
Please guard this log line with a verbosity check of 2.
|
|
||
| // implements the subsetting algorithm, as described in A68: https://github.com/grpc/proposal/pull/423 | ||
| func (b *subsettingBalancer) prepareChildResolverState(s resolver.State) resolver.State { | ||
| addresses := s.Addresses |
There was a problem hiding this comment.
We should be using the Endpoints field in resolver.State for everything here (except for computing the hash where we use the first address inside of the endpoint).
| // Convert back to resolver.addresses | ||
| addressesSubset := make([]resolver.Address, b.cfg.SubsetSize) | ||
| for _, eh := range addressesSet[:b.cfg.SubsetSize] { | ||
| addressesSubset = append(addressesSubset, eh.addr) | ||
| } | ||
|
|
||
| return resolver.State{ | ||
| Addresses: addressesSubset, | ||
| ServiceConfig: s.ServiceConfig, | ||
| Attributes: s.Attributes, | ||
| } |
There was a problem hiding this comment.
All of this should deal with endpoints and not addresses.
| func (b *subsettingBalancer) ResolverError(err error) { | ||
| b.child.ResolverError(err) | ||
| } | ||
|
|
||
| func (b *subsettingBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.SubConnState) { | ||
| b.child.UpdateSubConnState(sc, state) | ||
| } | ||
|
|
||
| func (b *subsettingBalancer) Close() { | ||
| b.child.Close() | ||
| } | ||
|
|
||
| func (b *subsettingBalancer) ExitIdle() { | ||
| b.child.ExitIdle() | ||
| } |
There was a problem hiding this comment.
Could we embed the gracefulswitch.Balancer in the subsettingBalancer, and thereby get rid of all these forwarding methods?
| ChildPolicy *iserviceconfig.BalancerConfig `json:"child_policy,omitempty"` | ||
| } | ||
|
|
||
| func (bb) ParseConfig(s json.RawMessage) (serviceconfig.LoadBalancingConfig, error) { |
There was a problem hiding this comment.
We need to add unit tests for this method. Thanks.
2 participants
Introduce a new LB policy,
random_subsetting. This policy selects a subset of addresses and passes them to the child LB policy to correct the resulting server-side load balancing.