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

clusterimpl: update picker synchronously on config update #7617

Closed

Conversation

aranjans
Copy link
Contributor

@aranjans aranjans commented Sep 11, 2024

#5469 recommends an audit of existing LB policies to ensure that they update their pickers synchronously upon receipt of a configuration update. clusterimpl does not update picker synchronously before because of multiple reasons, one is on reciept of configuration update, process of switching the child policies was aysnchronous, and second is it was not waiting for the child policy config to be update first, before returning from UpdateClientConnState.

What does this PR do?
This PR ensures that every time configuration update is triggered, picker is updated synchronously.

RELEASE NOTES:

  • clusterimpl: update picker synchronously on receipt of configuration update.

@aranjans aranjans added this to the 1.67 Release milestone Sep 11, 2024
@aranjans aranjans added Type: Bug Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. labels Sep 11, 2024
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.92%. Comparing base (8f920c6) to head (dd31a8b).
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7617      +/-   ##
==========================================
- Coverage   82.01%   81.92%   -0.09%     
==========================================
  Files         361      361              
  Lines       27813    27819       +6     
==========================================
- Hits        22812    22792      -20     
- Misses       3822     3839      +17     
- Partials     1179     1188       +9     
Files with missing lines Coverage Δ
xds/internal/balancer/clusterimpl/clusterimpl.go 90.78% <100.00%> (+0.26%) ⬆️

... and 16 files with indirect coverage changes

@aranjans aranjans force-pushed the aranjans_clusterimpl_picker_update_1 branch from 88b76a4 to 48dd6b2 Compare September 11, 2024 06:58
@aranjans aranjans modified the milestones: 1.67 Release, 1.68 Release Sep 11, 2024
@aranjans aranjans changed the title clusterimpl: stop using gs.switchTo on child policy config update clusterimpl: update picker synchronously on config update Sep 11, 2024
@easwars easwars assigned easwars and unassigned aranjans Sep 18, 2024
@easwars
Copy link
Contributor

easwars commented Sep 18, 2024

Can we please ensure that any outstanding comments on #7567 are handled, and that the PR is merged before switching back to this.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants