Responded to Doug's comments

grpc · zasweq · Apr 17, 2024 · Oct 3, 2023 · Mar 27, 2024 · Mar 28, 2024
commit c6bc43489ffa75f5484dead3074beade69c5b483
diff --git a/examples/features/customloadbalancer/README.md b/examples/features/customloadbalancer/README.md
@@ -1,6 +1,6 @@
 # Custom Load Balancer
 
-This examples shows how to deploy a custom load balancer in a `ClientConn`.
+This example shows how to deploy a custom load balancer in a `ClientConn`.
 
 ## Try it
 
@@ -14,24 +14,30 @@ go run client/main.go
 
 ## Explanation
 
-Two echo servers are serving on "localhost:20000" and "localhost:20001". They will include their
-serving address in the response. So the server on "localhost:20001" will reply to the RPC
-with `this is examples/customloadbalancing (from localhost:20001)`.
+Two echo servers are serving on "localhost:20000" and "localhost:20001". They
+will include their serving address in the response. So the server on
+"localhost:20001" will reply to the RPC with `this is
+examples/customloadbalancing (from localhost:20001)`.
 
-A client is created, to connect to both of these servers (they get both
-server addresses from the name resolver in two separate endpoints). The client is configured with the load
-balancer specified in the service config, which in this case is custom_round_robin.
+A client is created, to connect to both of these servers (they get both server
+addresses from the name resolver in two separate endpoints). The client is
+configured with the load balancer specified in the service config, which in this
+case is custom_round_robin.
 
 ### custom_round_robin
 
-The client is configured to use `custom_round_robin`. `custom_round_robin` is a petiole policy,
-which creates a pick first child for every endpoint it receives. It waits until both pick first children
-become ready, then defers to the first pick first child's picker, choosing the connection to localhost:20000, except
-every n times, where it defers to second pick first child's picker, choosing the connection to localhost:20001.
-
-`custom_round_robin` is written as a petiole policy wrapping `pick_first` load balancers, one for every endpoint received.
-This is the intended way a user written custom lb should be specified, as pick first will contain a lot of useful
-functionlaity, such as Sticky Transient Failure, Happy Eyeballs, and Health Checking.
+The client is configured to use `custom_round_robin`. `custom_round_robin`
+creates a pick first child for every endpoint it receives. It waits until both
+pick first children become ready, then defers to the first pick first child's
+picker, choosing the connection to localhost:20000, except every chooseSecond
+times, where it defers to second pick first child's picker, choosing the
+connection to localhost:20001.
+
+`custom_round_robin` is written as a delegating policy wrapping `pick_first`
+load balancers, one for every endpoint received. This is the intended way a user
+written custom lb should be specified, as pick first will contain a lot of
+useful functionality, such as Sticky Transient Failure, Happy Eyeballs, and
+Health Checking.
 
 ```
 this is examples/customloadbalancing (from localhost:20000)

diff --git a/examples/features/customloadbalancer/client/customroundrobin/customroundrobin.go b/examples/features/customloadbalancer/client/customroundrobin/customroundrobin.go
@@ -40,16 +40,16 @@ const customRRName = "custom_round_robin"
 type customRRConfig struct {
 	serviceconfig.LoadBalancingConfig `json:"-"`
 
-	// N represents how often pick iterations chose the second SubConn in the
-	// list. Defaults to 3. If 0 never choses second SubConn.
-	N uint32 `json:"n,omitempty"`
+	// ChooseSecond represents how often pick iterations choose the second
+	// SubConn in the list. Defaults to 3. If 0 never choose the second SubConn.
+	ChooseSecond uint32 `json:"chooseSecond,omitempty"`
 }
 
 type customRoundRobinBuilder struct{}
 
 func (customRoundRobinBuilder) ParseConfig(s json.RawMessage) (serviceconfig.LoadBalancingConfig, error) {
 	lbConfig := &customRRConfig{
-		N: 3,
+		ChooseSecond: 3,
 	}
 	if err := json.Unmarshal(s, lbConfig); err != nil {
 		return nil, fmt.Errorf("custom-round-robin: unable to unmarshal customRRConfig: %v", err)
@@ -78,7 +78,7 @@ var logger = grpclog.Component("example")
 
 type customRoundRobin struct {
 	// All state and operations on this balancer are either initialized at build
-	// time and read only after, or are only accessed as part of it's
+	// time and read only after, or are only accessed as part of its
 	// balancer.Balancer API (UpdateState from children only comes in from
 	// balancer.Balancer calls as well, and children are called one at a time),
 	// in which calls are guaranteed to come synchronously. Thus, no extra
@@ -92,27 +92,26 @@ type customRoundRobin struct {
 	pickFirstBuilder balancer.Builder
 	pfs              *resolver.EndpointMap
 
-	n                    uint32
+	cfg *customRRConfig
+
+	// InhibitPickerUpdates determines whether picker updates from the child
+	// forward to cc or not.
 	inhibitPickerUpdates bool
 }
 
 func (crr *customRoundRobin) UpdateClientConnState(state balancer.ClientConnState) error {
-	if logger.V(2) {
-		logger.Info("custom_round_robin: got new ClientConn state: ", state)
-	}
 	crrCfg, ok := state.BalancerConfig.(*customRRConfig)
 	if !ok {
 		return balancer.ErrBadResolverState
 	}
-	crr.n = crrCfg.N
+	crr.cfg = crrCfg
 
 	endpointSet := resolver.NewEndpointMap()
 	crr.inhibitPickerUpdates = true
 	for _, endpoint := range state.ResolverState.Endpoints {
 		endpointSet.Set(endpoint, nil)
 		var pickFirst *balancerWrapper
-		pf, ok := crr.pfs.Get(endpoint)
-		if ok {
+		if pf, ok := crr.pfs.Get(endpoint); ok {
 			pickFirst = pf.(*balancerWrapper)
 		} else {
 			pickFirst = &balancerWrapper{
@@ -147,7 +146,7 @@ func (crr *customRoundRobin) UpdateClientConnState(state balancer.ClientConnStat
 		}
 	}
 	crr.inhibitPickerUpdates = false
-	crr.regeneratePicker() // one synchronous picker update per Update Client Conn State operation.
+	crr.regeneratePicker() // one synchronous picker update per UpdateClientConnState operation.
 	return nil
 }
 
@@ -199,9 +198,9 @@ func (crr *customRoundRobin) regeneratePicker() {
 		return
 	}
 	picker := &customRoundRobinPicker{
-		pickers: readyPickers,
-		n:       crr.n,
-		next:    0,
+		pickers:      readyPickers,
+		chooseSecond: crr.cfg.ChooseSecond,
+		next:         0,
 	}
 	crr.cc.UpdateState(balancer.State{
 		ConnectivityState: connectivity.Ready,
@@ -233,15 +232,15 @@ func (bw *balancerWrapper) UpdateState(state balancer.State) {
 }
 
 type customRoundRobinPicker struct {
-	pickers []balancer.Picker
-	n       uint32
-	next    uint32
+	pickers      []balancer.Picker
+	chooseSecond uint32
+	next         uint32
 }
 
 func (crrp *customRoundRobinPicker) Pick(info balancer.PickInfo) (balancer.PickResult, error) {
 	next := atomic.AddUint32(&crrp.next, 1)
 	index := 0
-	if next != 0 && next%crrp.n == 0 {
+	if next != 0 && next%crrp.chooseSecond == 0 {
 		index = 1
 	}
 	childPicker := crrp.pickers[index%len(crrp.pickers)]

diff --git a/examples/features/customloadbalancer/client/main.go b/examples/features/customloadbalancer/client/main.go
@@ -46,7 +46,7 @@ func main() {
 	// You can also plug in your own custom lb policy, which needs to be
 	// configurable. This n is configurable. Try changing n and see how the
 	// behavior changes.
-	json := `{"loadBalancingConfig": [{"custom_round_robin":{"n": 3}}]}`
+	json := `{"loadBalancingConfig": [{"custom_round_robin":{"chooseSecond": 3}}]}`
 	sc := internal.ParseServiceConfig.(func(string) *serviceconfig.ParseResult)(json)
 	mr.InitialState(resolver.State{
 		Endpoints: []resolver.Endpoint{

diff --git a/pickfirst.go b/pickfirst.go
@@ -130,8 +130,7 @@ func (b *pickfirstBalancer) UpdateClientConnState(state balancer.ClientConnState
 	}
 
 	var addrs []resolver.Address
-	if len(state.ResolverState.Endpoints) != 0 {
-		endpoints := state.ResolverState.Endpoints
+	if endpoints := state.ResolverState.Endpoints; len(endpoints) != 0 {
 		// Perform the optional shuffling described in gRFC A62. The shuffling will
 		// change the order of endpoints but not touch the order of the addresses
 		// within each endpoint. - A61
@@ -145,17 +144,16 @@ func (b *pickfirstBalancer) UpdateClientConnState(state balancer.ClientConnState
 		for _, endpoint := range endpoints {
 			// "In the flattened list, interleave addresses from the two address
 			// families, as per RFC-8304 section 4." - A61
-			// This language is handled by this iteration through endpoints, as ipv4
-			// and ipv6 are specified as part of the same endpoint.
+			// TODO: support the above language.
 			addrs = append(addrs, endpoint.Addresses...)
 		}
+	} else {
 		// Endpoints not set, process addresses until migrate resolver emissions
 		// fully to Endpoints. The top channel does wrap emitted addresses with
 		// endpoints, however some balancers such as weighted target do not forwarrd
 		// the corresponding correct endpoints down/split endpoints properly. Once
 		// all balancers correctly forward endpoints down, can delete this else
 		// conditional.
-	} else {
 		addrs = state.ResolverState.Addresses
 		if cfg.ShuffleAddressList {
 			addrs = append([]resolver.Address{}, addrs...)