Repository: grpc/grpc-go
File: balancer/weightedroundrobin/balancer.go (Go)

Code before:

func (b *wrrBalancer) NewSubConn(addrs []resolver.Address, opts balancer.NewSubConnOptions) (balancer.SubConn, error) {
	addr := addrs[0] // The new pick first policy for DualStack will only ever create a SubConn with one address.
	var sc balancer.SubConn

	oldListener := opts.StateListener
	opts.StateListener = func(state balancer.SubConnState) {
		b.updateSubConnState(sc, state)
		oldListener(state)
	}

	b.mu.Lock()
	defer b.mu.Unlock()
	ewi, ok := b.addressWeights.Get(addr)
	if !ok {
		// SubConn state updates can come in for a no longer relevant endpoint
		// weight (from the old system after a new config update is applied).
		return nil, fmt.Errorf("wrr: balancer is being closed; no new SubConns allowed")
	}
	sc, err := b.ClientConn.NewSubConn([]resolver.Address{addr}, opts)
	if err != nil {
		return nil, err
	}
	if b.logger.V(2) {
		b.logger.Infof("Created SubConn %v for address %+v", sc, addr)
	}
	b.scToWeight[sc] = ewi
	return sc, nil
}

func (b *wrrBalancer) ResolverError(err error) {
	if b.logger.V(2) {
		b.logger.Infof("Received error from resolver: %v", err)
	}
	// Will cause inline picker update from endpoint sharding.
	b.child.ResolverError(err)
}

func (b *wrrBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.SubConnState) {
	b.logger.Errorf("UpdateSubConnState(%v, %+v) called unexpectedly", sc, state)
}

func (b *wrrBalancer) updateSubConnState(sc balancer.SubConn, state balancer.SubConnState) {
	if b.logger.V(2) {
		b.logger.Infof("Received update from SubConn %v with state: %+v", sc, state)
	}

	b.mu.Lock()
	ew := b.scToWeight[sc]
	// updates from a no longer relevant SubConn update, nothing to do here but
	// forward state to state listener, which happens in wrapped listener. Will

Review comment (suggestion):
This `NewSubConn` call is a passthrough in WRR. The call is initiated by the leaf pickfirst balancer and we don't expect the address to change. Do you think it makes sense to log the same message in every LB policy b/w pickfirst and the channel? In my opinion, it may not provide enough value for the noise that will be added.

Code after:

func (b *wrrBalancer) NewSubConn(addrs []resolver.Address, opts balancer.NewSubConnOptions) (balancer.SubConn, error) {
	addr := addrs[0] // The new pick first policy for DualStack will only ever create a SubConn with one address.
	var sc balancer.SubConn

	oldListener := opts.StateListener
	opts.StateListener = func(state balancer.SubConnState) {
		b.updateSubConnState(sc, state)
		oldListener(state)
	}

	b.mu.Lock()
	defer b.mu.Unlock()
	ewi, ok := b.addressWeights.Get(addr)
	if !ok {
		// SubConn state updates can come in for a no longer relevant endpoint
		// weight (from the old system after a new config update is applied).
		return nil, fmt.Errorf("wrr: balancer is being closed; no new SubConns allowed")
	}
	sc, err := b.ClientConn.NewSubConn([]resolver.Address{addr}, opts)
	if err != nil {
		return nil, err
	}
	b.scToWeight[sc] = ewi
	return sc, nil
}

func (b *wrrBalancer) ResolverError(err error) {
	if b.logger.V(2) {
		b.logger.Infof("Received error from resolver: %v", err)
	}
	// Will cause inline picker update from endpoint sharding.
	b.child.ResolverError(err)
}

func (b *wrrBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.SubConnState) {
	b.logger.Errorf("UpdateSubConnState(%v, %+v) called unexpectedly", sc, state)
}

func (b *wrrBalancer) updateSubConnState(sc balancer.SubConn, state balancer.SubConnState) {
	if b.logger.V(2) {
		b.logger.Infof("Received update from SubConn %v with state: %+v", sc, state)
	}

	b.mu.Lock()
	ew := b.scToWeight[sc]
	// updates from a no longer relevant SubConn update, nothing to do here but
	// forward state to state listener, which happens in wrapped listener. Will
	// eventually get cleared from scMap once receives Shutdown signal.
	if ew == nil {
		b.mu.Unlock()