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/priority: bug fix and minor behavior change #5417

Merged
merged 2 commits into from Jun 17, 2022

Conversation

dfawley
Copy link
Contributor

@dfawley dfawley commented Jun 10, 2022

#5211

  • Bug: push config updates to all child policies, not just the active one.
  • Process all child picker updates before determining active priority.

Logic changes are all in .../balancer/priority/*.go; the other changes are tests that are too dependent on the priority balancer's behavior (not correctness related; just not allowing for the possibility of duplicate picker updates).

RELEASE NOTES:

  • xds/priority: fix bug that could prevent higher priorities from receiving config updates

- Bug: push config updates to all child policies, not just the active one.
- Process all child picker updates before determining active priority.
@dfawley dfawley added this to the 1.48 Release milestone Jun 10, 2022
@dfawley dfawley requested a review from zasweq June 10, 2022 20:39
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

First PR done! I'm super proud of my comments :D.

xds/internal/balancer/priority/balancer.go Outdated Show resolved Hide resolved
xds/internal/balancer/priority/balancer.go Show resolved Hide resolved
// an update of the aggregated picker to the parent. Cleared after all
// sub-balancers have finished UpdateClientConnState, after which
// syncPriority is called manually.
inhibitChildUpdates bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I don't like this name. We are inhibiting the child picker updates, but we are inhibiting the calculation/synchronization of the priority, which determines what child picker update to send back. Maybe inhibitSyncPriority?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

inhibitPickerUpdates?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sure, even though syncPriority calls aren't exactly 1:1 with a picker update, you're still inhibiting all of the possible picker updates, even though that's only a fraction of the time syncPriority is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

syncPriority is the only thing that pushes a picker update to the parent policy, which is how I was thinking of this (and how the functionality was debated/designed cross-language).

xds/internal/balancer/clusterimpl/balancer_test.go Outdated Show resolved Hide resolved
xds/internal/balancer/clusterimpl/balancer_test.go Outdated Show resolved Hide resolved
xds/internal/balancer/clusterimpl/balancer_test.go Outdated Show resolved Hide resolved
@zasweq zasweq assigned dfawley and unassigned zasweq Jun 14, 2022
@dfawley dfawley assigned zasweq and unassigned dfawley Jun 16, 2022
zasweq
zasweq approved these changes Jun 17, 2022
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

LGTM! Now I need to go watch the Warriors win a championship.

}

// Two new subconns are created by the previous update; one by p0 and one
// by p1. They don't happen concurrently, but they could happen in any
Copy link
Contributor

Choose a reason for hiding this comment

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

Nondeterministic map iterations :0.

xds/internal/balancer/priority/balancer.go Outdated Show resolved Hide resolved
// an update of the aggregated picker to the parent. Cleared after all
// sub-balancers have finished UpdateClientConnState, after which
// syncPriority is called manually.
inhibitChildUpdates bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sure, even though syncPriority calls aren't exactly 1:1 with a picker update, you're still inhibiting all of the possible picker updates, even though that's only a fraction of the time syncPriority is called.

@zasweq zasweq assigned dfawley and unassigned zasweq Jun 17, 2022
@dfawley dfawley changed the title priority: bug fix and minor behavior change xds/priority: bug fix and minor behavior change Jun 17, 2022
@dfawley dfawley merged commit 3e7b97f into grpc:master Jun 17, 2022
11 checks passed
@dfawley dfawley deleted the updatepicker branch June 17, 2022 18:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants