Skip to content

Conversation

@temawi
Copy link
Contributor

@temawi temawi commented Apr 4, 2023

Currently the code maintains one LoadStatsManager2 that collects all stats. The problem with this is that in a federation situation there will be multiple LrsClients that will be periodically picking up stats from the manager and sending them to their respective control planes. This creates a first-come-first-serve situation where the stats get randomly distributed across the control planes.

This change creates separate LoadStatsManagers dedicated to their own control planes, thus assuring no stats will get lost.

b/274812461

Currently the code maintains one LoadStatsManager2 that collects all
stats. The problem with this is that in a federation situation there
will be multiple LrsClients that will be periodically picking up stats
from the manager and sending them to their respective control planes.
This creates a first-come-first-serve situation where the stats get
randomly distributed across the control planes.

This change creates separate LoadStatsManagers dedicated to their own
control planes, thus assuring no stats will get lost.
@temawi temawi requested a review from YifeiZhuang April 4, 2023 02:58
@temawi
Copy link
Contributor Author

temawi commented Apr 4, 2023

cc: @larry-safran

@temawi
Copy link
Contributor Author

temawi commented Apr 4, 2023

While this change assures no stats get missed, it will send all stats for all control planes to every control plane. A possibly more correct approach would be to filter out stats that are not associated with a given control plane, but this could be done as a follow-up.

@temawi temawi merged commit 6d75fca into grpc:master Apr 4, 2023
@temawi temawi deleted the distinct-load-stats-managers branch April 4, 2023 18:29
@larry-safran
Copy link
Contributor

larry-safran commented Apr 4, 2023 via email

@temawi
Copy link
Contributor Author

temawi commented Apr 5, 2023

Shouldn't there be null checks on the loadStatsManager since it is now retrieved by a Map.get()?

As discussed offline, the loadStatsManager is populated in the map when the connection to the control plane is initially set up (in maybeCreateXdsChannelWithLrs()) and since we would only access a loadStatsManager after name resolution has happened (which needs a control plane connection), the map should already been populated for the control plane.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants