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

rls: update picker synchronously on configuration update #7412

Merged
merged 15 commits into from
Aug 16, 2024

Conversation

aranjans
Copy link
Contributor

@aranjans aranjans commented Jul 14, 2024

fixes #7212.

Problem

#5469 recommends an audit of existing LB policies to ensure that they update their pickers synchronously upon receipt of a configuration update. RLS lb policy do updates the picker synchronously but misses out in one single corner case when there is a config change in cache size, where we need to send picker update in case any cache entry with valid backoff was evicted.

What does this PR do?

This PR handles that corner cases and make sure it updates the picker in all of the cases synchronously with config update of RLS lb policy.

RELEASE NOTES:

  • rls: update picker synchronously on configuration update.

@aranjans aranjans added this to the 1.66 Release milestone Jul 14, 2024
@aranjans aranjans changed the title rls: update picker synchronously upon receipt of configuration update rls: update picker synchronously on configuration update. Jul 14, 2024
Copy link

codecov bot commented Jul 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.82%. Comparing base (c98235b) to head (09c0dea).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7412      +/-   ##
==========================================
- Coverage   81.96%   81.82%   -0.15%     
==========================================
  Files         360      360              
  Lines       27533    27533              
==========================================
- Hits        22568    22529      -39     
- Misses       3780     3799      +19     
- Partials     1185     1205      +20     
Files Coverage Δ
balancer/rls/balancer.go 85.81% <100.00%> (ø)

... and 24 files with indirect coverage changes

Copy link
Contributor

@arjan-bal arjan-bal left a comment

Choose a reason for hiding this comment

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

Great progress since the last attempt!

balancer/rls/balancer_test.go Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Show resolved Hide resolved
balancer/rls/balancer.go Outdated Show resolved Hide resolved
@arjan-bal arjan-bal assigned aranjans and unassigned arjan-bal Jul 17, 2024
@aranjans
Copy link
Contributor Author

aranjans commented Jul 18, 2024

@arjan-bal I have updated the PR and referenced your comments in the code change.
fyi: i have removed the additional listener.

balancer/rls/balancer.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
@arjan-bal arjan-bal assigned aranjans and unassigned arjan-bal Jul 18, 2024
@aranjans
Copy link
Contributor Author

@arjan-bal thAnks for your review, I have updated the PR with your suggestions.
fyi: For all the suggestions, I have just put the thumbs up to the comment and updated the PR.

@aranjans aranjans assigned arjan-bal and unassigned aranjans Jul 19, 2024
balancer/rls/balancer.go Outdated Show resolved Hide resolved
@aranjans aranjans assigned aranjans and easwars and unassigned easwars and aranjans Aug 13, 2024
balancer/rls/balancer.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
balancer/rls/balancer_test.go Outdated Show resolved Hide resolved
@easwars easwars assigned aranjans and unassigned easwars Aug 14, 2024
@aranjans aranjans assigned easwars and unassigned aranjans Aug 14, 2024
balancer/rls/balancer.go Outdated Show resolved Hide resolved
@easwars easwars assigned aranjans and unassigned easwars Aug 15, 2024
@aranjans aranjans assigned easwars and unassigned aranjans Aug 16, 2024
@easwars easwars assigned aranjans and unassigned easwars Aug 16, 2024
@easwars easwars merged commit 63853fd into grpc:master Aug 16, 2024
13 checks passed
infovivek2020 pushed a commit to infovivek2020/grpc-go that referenced this pull request Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rls: update picker synchronously upon receipt of configuration update
4 participants