lrs: add Store.Stats() to report loads for multiple clusters #3905

menghanl · 2020-09-25T00:14:29Z

unexport perClusterStore and it's stats()
add Store.Stats(clusterNames) to report loads for the given clusters
- refactor store's map to a two layer map
move lastLoadReportAt from client ton the load store, because a client can now have multiple clusters, each with a different lastLoadReportAt
- all tests will ignore ReportInterval when comparing Data

…lusters - unexport PerClusterStore.Stats() - add Store.Stats(clusterNames) to report loads for the given clusters

easwars · 2020-09-29T22:22:46Z

xds/internal/client/load/store.go

+// a slice with no specific order.
+//
+// If no clusterName is given (an empty slice), all data for all known clusters
+// are returned.


easwars · 2020-09-29T22:23:07Z

xds/internal/client/load/store.go

+// If no clusterName is given (an empty slice), all data for all known clusters
+// are returned.
+//
+// If a cluster's Data is empty (no load to report), it's not append to the


s/append/appended/

easwars · 2020-09-29T22:25:48Z

xds/internal/client/load/store.go

+func appendClusterStats(ret []*Data, cluster map[string]*perClusterStore) []*Data {
+ for _, d := range cluster {
+ data := d.stats()
+ if data.TotalDrops == 0 && len(data.Drops) == 0 && len(data.LocalityStats) == 0 {


how about adding a method on Data to return if its empty?

I changed d.stats() to return nil if data doesn't contain any information.

easwars · 2020-09-29T22:37:36Z

xds/internal/client/load/store.go

+// the new slice.
+//
+// Data is only appended to ret if it's not empty.
+func appendClusterStats(ret []*Data, cluster map[string]*perClusterStore) []*Data {


This could be minor thing. I feel that instead of appendClusterStats which takes a slice and appends to it and returns the appended slice, we could simply have fetchClusterStats which just returns a slice of Data for the specified cluster. The caller can do what it wants with the returned slice.

Or another option would be to get rid of this function and fold it into Stats. And in there, first figure out the actual list of cluster names we are interested in (either all, or the provided list minus the ones for whcih there is no entry in the clusters map).

I didn't find a cleaner way to remove this function while handling special case len(clusters)==0..

easwars · 2020-09-29T22:45:46Z

xds/internal/client/load/store.go

- clusters map[storeKey]*PerClusterStore
+ // mu only protects the map (2 layers). The read/write to *perClusterStore
+ // doesn't need to hold the mu.
+ mu sync.RWMutex


Is a read-write mutex really required here?

I see that the loadStoreWrapper in xds_client_wrapper.go actually invokes the PerCluster method for each of it Call* methods. Could we change that to instead store the PerClusterReporter locally as part of its update method?

Updated the caller to store PerClusterReporter locally.

Kept the RWMutex, because it could still be use in some cases.

The RWMutex just makes the PerCluster() method harder to read. Unless there is a good reason for using it, we probably shouldn't.

Changed to just Mutex. Since perCluster is cached, we wouldn't need to take this lock a lot.

easwars · 2020-09-29T22:48:41Z

xds/internal/client/load/store.go

@@ -87,16 +145,20 @@ func (ls *Store) PerCluster(clusterName, serviceName string) *PerClusterStore {
 // RWMutex.
 // Neither of these conditions are met here, and we should transition to a
 // regular map with a mutex for better type safety.
-type PerClusterStore struct {
+type perClusterStore struct {
+ cluster, service string


Storing things like cluster, service and lastLoadReportAt does not seem right here. I find no reason for the perClusterStore or the returned Data to contain these things. These should be ideally kept track of by the client which is eventually reporting stuff to the LRS server.

This is just for convenience. Store already has a map like what you said. But we will need another wrapper to keep lastLoadReportAt.

Another difference is whether Store.Stats() returns a map (map[cluster]map[service]*Data), or a list of key-value pairs ([]*Data).

easwars · 2020-09-29T23:03:29Z

xds/internal/client/load/store.go

+ // with service name as the key. Each value (perClusterStore) contains data
+ // for a (cluster, service) pair.
+ //
+ // Note that new entries are added to this map, but never removed. This is


Is there a way to avoid this? Just like how the users of the xdsClient need to cancel their watches, they are also required to call the cancel function returned from ReportLoad. Could we use that somehow?

There's no "correct" time to cancel reporting for a cluster,service pair. Because even when cluster,service is updated in the parent balancer, we cannot cancel reporting for the old pair until we know the sub-balancers has processed the new addresses and updated pickers to stop using the old subconns.
This is impossible with the current structure with sub-balancers. But will be possible when we pass attach ``cluster,service` to each address, and do all load reporting work inside CDS.

We may have a way to do the cleanup when CDS does all the tracking and load reporting.

easwars · 2020-09-29T23:11:18Z

xds/internal/client/load/store.go

+//
+// If a cluster's Data is empty (no load to report), it's not append to the
+// returned slice.
+func (s *Store) Stats(clusterNames []string) []*Data {


I'm wondering if this top-level Store should export any methods at all, except for the ones to retrieve the per-cluster store.

Also, I'm wondering whether we should fix the xdsClient API to support reporting loads to multiple clusters in a better way. Things on top of my head (without a lot of serious thought):

ReportLoad which takes (cluster, service) should return the PerClusterStore and a cancel function

Callers use the returned PerClusterStore to report loads, and eventually invoke the cancel function

Internally, the xdsClient keeps track of every cluster+service combination for which it is reporting load, and the associated PerClusterStore

When it is time to report load (every loadReporting internal), it traverses the list of PerClusterStore and fetches stats from them and creates the load report and sends it to the server.

This is similar to what I have in the follow up PR, see https://github.com/menghanl/grpc-go/pull/43/files#diff-5f29997be1764e4c5a06ac2f4d3d1bb7R51

Difference:

ReportLoad doesn't take cluster, service. It only takes the server address. The store for each server address can be reused to get the per-cluster load reporter

There's no "correct" time to cancel the load report for the cluster, service combo. But we can just stop reporting to the old server when EDS contains a different server name.

easwars · 2020-10-05T15:41:16Z

xds/internal/balancer/edsbalancer/xds_client_wrapper.go

 }

 func (lsw *loadStoreWrapper) update(store *load.Store, service string) {
 lsw.mu.Lock()
 defer lsw.mu.Unlock()
+ if store == lsw.store && service == lsw.service {


Not convinced if we need this check here and in the lrs/balancer.go.

Do you mean we don't need this check? Or we should move this check somewhere else?

The purpose of this check is to use the cached perCluster to avoid getting the lock inside store. This update can happen frequently, with the same parameters.

I thought we don't need this check because I thought this update will not happen frequently. Why would this update happen frequently with the same parameters?

This is called on each received CDS response. (Not that frequently comparing to RPCs.)

A normal unnecessary (from lrs's point of view) update would be, something else (the clusters) in CDS changed, but the lrs-server string doesn't.
A not-so-normal unnecessary update is when the server sends the same CDS resp multiple times.

easwars · 2020-10-05T15:52:23Z

xds/internal/client/load/store.go

- clusters map[storeKey]*PerClusterStore
+ // mu only protects the map (2 layers). The read/write to *perClusterStore
+ // doesn't need to hold the mu.
+ mu sync.RWMutex


The RWMutex just makes the PerCluster() method harder to read. Unless there is a good reason for using it, we probably shouldn't.

easwars · 2020-10-05T15:54:07Z

xds/internal/client/load/store.go

- ls.mu.Lock()
- defer ls.mu.Unlock()
- p, ok := ls.clusters[k]
+ p, ok := c[serviceName]


Maybe:

if p, ok := c[serviceName]; ok { return p } p = &perClusterStore { ... } c[serviceName] = p return p

[lrs_stream] lrs: add Stats() to store to report loads for multiple c…

2bd5181

…lusters - unexport PerClusterStore.Stats() - add Store.Stats(clusterNames) to report loads for the given clusters

menghanl requested a review from easwars September 25, 2020 00:14

menghanl assigned easwars Sep 25, 2020

menghanl added no release notes Type: Feature New features or improvements in behavior labels Sep 25, 2020

easwars requested changes Sep 29, 2020

View reviewed changes

easwars assigned menghanl and unassigned easwars Sep 29, 2020

menghanl added 2 commits October 1, 2020 10:59

[lrs_stream] c0

72d6f74

[lrs_stream] c1

dbe4edd

dfawley changed the title ~~lrs: add Stats() to store to report loads for multiple clusters~~ lrs: add Stats() to Store to report loads for multiple clusters Oct 1, 2020

dfawley assigned easwars and unassigned menghanl Oct 1, 2020

easwars approved these changes Oct 5, 2020

View reviewed changes

easwars assigned menghanl and unassigned easwars Oct 5, 2020

[lrs_stream] c2

ae8ecc2

menghanl changed the title ~~lrs: add Stats() to Store to report loads for multiple clusters~~ lrs: add Store.Stats() to report loads for multiple clusters Oct 7, 2020

menghanl merged commit 5af6040 into grpc:master Oct 7, 2020

menghanl deleted the lrs_stream branch October 7, 2020 17:40

github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lrs: add Store.Stats() to report loads for multiple clusters #3905

lrs: add Store.Stats() to report loads for multiple clusters #3905

menghanl commented Sep 25, 2020

easwars Sep 29, 2020

menghanl Oct 1, 2020

easwars Sep 29, 2020

menghanl Oct 1, 2020

easwars Sep 29, 2020

menghanl Oct 1, 2020

easwars Sep 29, 2020

menghanl Oct 1, 2020

easwars Sep 29, 2020

menghanl Oct 1, 2020

easwars Oct 5, 2020

menghanl Oct 6, 2020

easwars Sep 29, 2020

menghanl Oct 1, 2020

easwars Sep 29, 2020

menghanl Oct 1, 2020

easwars Sep 29, 2020

menghanl Oct 1, 2020

easwars Oct 5, 2020

menghanl Oct 6, 2020

easwars Oct 6, 2020

menghanl Oct 6, 2020

easwars Oct 5, 2020

easwars Oct 5, 2020

menghanl Oct 6, 2020

lrs: add Store.Stats() to report loads for multiple clusters #3905

lrs: add Store.Stats() to report loads for multiple clusters #3905

Conversation

menghanl commented Sep 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment