lrs: add a layer for clusters in load store #3880
Conversation
|
Looks like vet isn't happy. |
| package load | ||
|
|
||
| // PerClusterReporter wraps the methods from the loadStore that are used here. | ||
| type PerClusterReporter interface { |
There was a problem hiding this comment.
Why do we want to define the interface here instead of at places where they are being used?
There was a problem hiding this comment.
Before, load.Store is passed around. Balancers/pickers constructors take load.Store as parameter, and assign it to unexported interfaces for the part they need.
Now, there's load.Store for all clusters, but each balancer only needs the PerClusterStore, so they need to wrap load.Store in another struct.
The constructors cannot take PerClusterStore because it's not the wrapper, not this type.
So we need this interface for the constructors to use.
There was a problem hiding this comment.
I think if we get rid of the loadStoreWrapper type, we would not need this. Although, I still don't clearly understand why we need this (even with the current state of things).
There was a problem hiding this comment.
We need a type for balancer.New to use. It used to be *load.Store. Now it cannot be *load.PerClusterStore, because the type is the wrapper.
The reason we need the wrapper is explained in the other comment.
dfawley
changed the title
| return nil | ||
| } | ||
| return c.xdsClient.LoadStore() | ||
| // return c.xdsClient.LoadStore().PerCluster(c.edsServiceName, "") |
|
|
||
| if restartEndpointsWatch { | ||
| c.startEndpointsWatch(nameToWatch) | ||
| if clientChanged || c.edsServiceName != config.EDSServiceName { |
There was a problem hiding this comment.
c.edsServiceName is only written to in startEndpointsWatch. Now that this is read from startLoadReport, I think it would be better to write to it here, rather than in startEndpointsWatch.
| @@ -45,6 +47,43 @@ var ( | |||
| bootstrapConfigNew = bootstrap.NewConfig | |||
| ) | |||
|
|
|||
| type loadStoreWrapper struct { | |||
There was a problem hiding this comment.
Do we need this type?
If we cached the PerClusterStore in the xdsClientWrapper whenever the LRS stream is to be updated (either the client changed, or the LRS server name changed or the edsServiceName changed), then it will simplify things, wouldn't it?
Also, I feel that caching the PerClusterStore would reduce lock contention, and the Store can also be simplified by not having to require a RWLock.
There was a problem hiding this comment.
When clusterName/serviceName is updated, the cached PerClusterStore needs to be updated, and passed to balancer group. But there's no way to update the load store in a balancer group.
| package load | ||
|
|
||
| // PerClusterReporter wraps the methods from the loadStore that are used here. | ||
| type PerClusterReporter interface { |
There was a problem hiding this comment.
I think if we get rid of the loadStoreWrapper type, we would not need this. Although, I still don't clearly understand why we need this (even with the current state of things).
- The original Store is renamed to PerClusterStore. The new Store has a method
to get PerClusterStore.
- All usages of old Store are updated to PerClusterStore
- In EDS and LRS, where the load reporting is happening (by wrapping the
picker), a closure is created with the store and clusterName, serviceName, so
loads will be associated with the right cluster.
- This also means when cluster/service is changed, the closure needs to
update as well.
- TODO: there's no correct timing for this update. The update in this change
is happening too early. The fix would be to do a graceful switch of EDS
when the cluster/service name is changed. That will happen in a later fix.
menghanl
force-pushed
the
lrs_store_per_cluster
branch
from
f030469
to
b648ef2
Compare
to get PerClusterStore.
picker), a closure is created with the store and clusterName, serviceName, so
loads will be associated with the right cluster.
update as well.
is happening too early. The fix would be to do a graceful switch of EDS
when the cluster/service name is changed. That will happen in a later fix.
and reports its data.
This change is