Skip to content
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

xds: use locality from the connected address for load reporting #7378

Merged
merged 9 commits into from
Jul 10, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Address review comments (2/x).
  • Loading branch information
townba committed Jul 8, 2024
commit 69acfcfacb9c31e5582b2723734fa3f39be29633
9 changes: 5 additions & 4 deletions balancer/balancer.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,10 @@ func unregisterForTesting(name string) {
delete(m, name)
}

// getConnectedAddress returns the connected address for a SubConnState and
// whether or not it is valid.
func getConnectedAddress(scs SubConnState) (resolver.Address, bool) {
// connectedAddress returns the connected address for a SubConnState.
// The second return value is set to to false if the state is not READY, and the
// first return value is meaningless in this case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is anyone even calling this if the state isn't ready? Can we just say it's only valid if READY to simplify things a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

func connectedAddress(scs SubConnState) (resolver.Address, bool) {
return scs.connectedAddress, scs.ConnectivityState == connectivity.Ready
}

Expand All @@ -85,7 +86,7 @@ func setConnectedAddress(scs *SubConnState, addr resolver.Address) {

func init() {
internal.BalancerUnregister = unregisterForTesting
internal.GetConnectedAddress = getConnectedAddress
internal.ConnectedAddress = connectedAddress
internal.SetConnectedAddress = setConnectedAddress
}

Expand Down
15 changes: 6 additions & 9 deletions clientconn.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"fmt"
"math"
"net/url"
"slices"
"strings"
"sync"
"sync/atomic"
Expand Down Expand Up @@ -918,22 +919,18 @@ func (ac *addrConn) connect() error {
return nil
}

// equalAddress returns true is a and b are considered equal.
// This is different from the Equal method on the resolver.Address type which
// considers all fields to determine equality. Here, we only consider fields
// that are meaningful to the subConn.
func equalAddress(a, b resolver.Address) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably makes sense to add a function level comment here saying why we are not using the resolver.Address.Equal and instead defining our own. Something as simple as:

// equalAddress returns true is a and b are considered equal.
// This is different from the Equal method on the resolver.Address type 
// which considers all fields to determine equality. Here, we only consider
// fields that are meaningful to the subConn.
func equalAddress(a, b resolver.Address) bool { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming the function might be an even nicer improvement. equalIgnoringBalAttributes?

Copy link
Contributor Author

@townba townba Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Used equalAddressIgnoringBalAttributes so I could also have equalAddressesIgnoringBalAttributes below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should probably be pointers to avoid the copy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return a.Addr == b.Addr && a.ServerName == b.ServerName &&
a.Attributes.Equal(b.Attributes) &&
a.Metadata == b.Metadata
}

func equalAddresses(a, b []resolver.Address) bool {
if len(a) != len(b) {
return false
}
for i, v := range a {
if !equalAddress(v, b[i]) {
return false
}
}
return true
return slices.EqualFunc(a, b, func(a, b resolver.Address) bool { return equalAddress(a, b) })
}

// updateAddrs updates ac.addrs with the new addresses list and handles active
Expand Down
7 changes: 4 additions & 3 deletions internal/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,10 @@ var (
// is the number of elements. swap swaps the elements with indexes i and j.
ShuffleAddressListForTesting any // func(n int, swap func(i, j int))

// GetConnectedAddress returns the connected address for a SubConnState and
// whether the address is valid based on the state.
GetConnectedAddress any // func (scs SubConnState) (resolver.Address, bool)
// ConnectedAddress returns the connected address for a SubConnState.
// The second return value is set to to false if the state is not READY, and the
// first return value is meaningless in this case.
ConnectedAddress any // func (scs SubConnState) (resolver.Address, bool)

// SetConnectedAddress sets the connected address for a SubConnState.
SetConnectedAddress any // func(scs *SubConnState, addr resolver.Address)
Expand Down
4 changes: 2 additions & 2 deletions xds/internal/balancer/clusterimpl/clusterimpl.go
Original file line number Diff line number Diff line change
Expand Up @@ -371,13 +371,13 @@
b.updateSubConnState(sc, state, oldListener)
// Read connected address and call updateLocalityID() based on the connected
// address's locality. https://github.com/grpc/grpc-go/issues/7339
if gca, ok := internal.GetConnectedAddress.(func(balancer.SubConnState) (resolver.Address, bool)); ok {
if gca, ok := internal.ConnectedAddress.(func(balancer.SubConnState) (resolver.Address, bool)); ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here -- remove the conditional and make this global, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if addr, ok := gca(state); ok {
lID := xdsinternal.GetLocalityID(addr)
if !lID.Empty() {
scw.updateLocalityID(lID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go style: let's try to avoid all this nesting:

StateListener = func... {
	b.updateSubConnState(...)
	if state != Ready {
		return
	}
	locality := xdsinternal.GetLocalityID(getConnectedAddress(state))
	if locality.Empty() {
		if logger.V(2) { log }
		return
	}
	scw.updateLocalityID(locality)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

} else if b.logger.V(2) {
b.logger.Infof("Locality ID for %v unexpectedly empty", addr)
b.logger.Infof("Locality ID for %s unexpectedly empty", addr)

Check warning on line 380 in xds/internal/balancer/clusterimpl/clusterimpl.go

View check run for this annotation

Codecov / codecov/patch

xds/internal/balancer/clusterimpl/clusterimpl.go#L380

Added line #L380 was not covered by tests
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion xds/internal/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func (l LocalityID) Equal(o any) bool {

// Empty returns whether or not the locality ID is empty.
func (l LocalityID) Empty() bool {
return len(l.Region) == 0 && len(l.Zone) == 0 && len(l.SubZone) == 0
return l.Region == "" && l.Zone == "" && l.SubZone == ""
}

// LocalityIDFromString converts a json representation of locality, into a
Expand Down
Loading