-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return nil | ||
} | ||
return c.xdsClient.LoadStore() | ||
// return c.xdsClient.LoadStore().PerCluster(c.edsServiceName, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
if restartEndpointsWatch { | ||
c.startEndpointsWatch(nameToWatch) | ||
if clientChanged || c.edsServiceName != config.EDSServiceName { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -45,6 +47,43 @@ var ( | |||
bootstrapConfigNew = bootstrap.NewConfig | |||
) | |||
|
|||
type loadStoreWrapper struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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