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: implement the top-level LB policy #7203

Merged
merged 13 commits into from Jul 20, 2020

Conversation

voidzcy
Copy link
Contributor

@voidzcy voidzcy commented Jul 13, 2020

The top-level LB policy, which is an aggregator for CDS policies. It maintains the lifecycle of CDS LB policy instances. The pick argument taken from the Channel contains the information to determine which child CDS policy instance should the picking operation be delegated to.

The implementation is similar to the action part of what we currently have in the routing policy. The existing routing policy will be refactored to two parts, with the route match part moved into ConfigSelector and action part being this top-level LB policy.

@voidzcy voidzcy force-pushed the impl/implement_top_level_lb_policy branch from 0a0960a to bcabf09 Compare July 16, 2020 17:37
class ClusterManagerLoadBalancer extends LoadBalancer {

@VisibleForTesting
static final int DELAYED_ACTION_DELETION_TIME_MINUTES = 15;
Copy link
Member

Choose a reason for hiding this comment

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

_ACTION_?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Fixed.


@Override
public void updateBalancingState(ConnectivityState newState, SubchannelPicker newPicker) {
if (deactivated) {
Copy link
Member

Choose a reason for hiding this comment

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

Even if deactivated, should still update currentState and currentPicker, just not updateOverallBalancingState() (already guaranteed). The XdsRoutingLoadBalancer has the same issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Fixed here.

this.helper = checkNotNull(helper, "helper");
this.syncContext = checkNotNull(helper.getSynchronizationContext(), "syncContext");
this.timeService = checkNotNull(helper.getScheduledExecutorService(), "timeService");
logger = XdsLogger.withLogId(InternalLogId.allocate("cds-lb", helper.getAuthority()));
Copy link
Member

Choose a reason for hiding this comment

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

cds-lb?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

syncContext.execute(new Runnable() {
@Override
public void run() {
childLbStates.get(name).lb.handleResolvedAddresses(childAddresses);
Copy link
Member

Choose a reason for hiding this comment

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

Declare variable for childLbStates.get(name) ahead of syncContext.execute(), otherwise childLbStates might have been mutated when `syncContext.execute() in extreme case.

Or just run childLbStates.get(name).lb.handleResolvedAddresses(childAddresses) inline at the end of this method.

Copy link
Contributor Author

@voidzcy voidzcy Jul 16, 2020

Choose a reason for hiding this comment

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

Good point, this turns out to be a programming practice. Thanks. Fixed. Will change it in XdsRoutingLoadBalancer as well (although it will go away after XdsConfigSelector, we want it to be correct in the release).

@voidzcy voidzcy merged commit b9d0676 into grpc:master Jul 20, 2020
dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021
The top-level LB policy, which is an aggregator for CDS policies. It maintains the lifecycle of CDS LB policy instances. The pick argument taken from the Channel contains the information to determine which child CDS policy instance should the picking operation be delegated to.

The implementation is similar to the action part of what we currently have in the routing policy. The existing routing policy will be refactored to two parts, with the route match part moved into ConfigSelector and action part being this top-level LB policy.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 10, 2021
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.

None yet

2 participants