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.

Sign up for GitHub

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

Jump to bottom

xds/clusterimpl: update UpdateClientConnState to handle updates synchronously #7533

Merged
merged 3 commits into from
Aug 30, 2024
Merged

xds/clusterimpl: update UpdateClientConnState to handle updates synchronously #7533

merged 3 commits into from
Aug 30, 2024

Conversation

aranjans
Copy link
Contributor

@aranjans aranjans commented Aug 19, 2024 •
edited by arvindbr8
Loading

Part of #7210.

UpdateClientConnState now handles updates in a blocking fashion.

To simplify the code, this PR replaces the existing run goroutine inside clusterimpl lb policy and instead handles calls from gRPC and from the child policy (to update state) in a grpcsync.CallbackSerializer. This approach simplifies things by removing the need for synchronization using locks and other grpcsync.Event fields.

RELEASE NOTES: none

@aranjans aranjans requested a review from easwars August 19, 2024 09:09
@aranjans aranjans added the Type: Internal Cleanup Refactors, etc label Aug 19, 2024
@aranjans aranjans added this to the 1.67 Release milestone Aug 19, 2024
@easwars easwars assigned aranjans and unassigned easwars Aug 20, 2024
@easwars
Copy link
Contributor

easwars commented Aug 20, 2024

I also updated the PR description and also got rid of the release notes since we don't need release notes for non-user visible behavior changes.

@codecov Codecov
Copy link

codecov bot commented Aug 21, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 90.62500% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.82%. Comparing base (b45fc41) to head (fc2054e).
Report is 24 commits behind head on master.

Files with missing lines Patch % Lines
xds/internal/balancer/clusterimpl/clusterimpl.go 90.62% 4 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7533      +/-   ##
==========================================
- Coverage   82.07%   81.82%   -0.26%     
==========================================
  Files         360      361       +1     
  Lines       27533    27808     +275     
==========================================
+ Hits        22599    22754     +155     
- Misses       3759     3853      +94     
- Partials     1175     1201      +26
Files with missing lines Coverage Δ
xds/internal/balancer/clusterimpl/clusterimpl.go 89.57% <90.62%> (+3.16%) ⬆️

... and 51 files with indirect coverage changes

@aranjans aranjans assigned easwars and unassigned aranjans Aug 22, 2024
@arvindbr8 arvindbr8 self-requested a review August 23, 2024 15:13
@arvindbr8 arvindbr8 assigned arvindbr8 and unassigned easwars Aug 23, 2024
arvindbr8
arvindbr8 approved these changes Aug 29, 2024
View reviewed changes
Copy link
Member

@arvindbr8 arvindbr8 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The core modification in this diff appears to be transitioning UpdateClientConnState into a blocking operation. We should explicitly mention this in the commit description.

Apart from a change suggested for a comment block, this diff LGTM.

xds/internal/balancer/clusterimpl/clusterimpl.go Outdated Show resolved Hide resolved
@arvindbr8 arvindbr8 assigned aranjans and unassigned arvindbr8 Aug 29, 2024
@easwars
Copy link
Contributor

easwars commented Aug 29, 2024

The core modification in this diff appears to be transitioning UpdateClientConnState into a blocking operation. We should explicitly mention this in the commit description.

UpdateClientConnState always needs to be a blocking operation since it needs to return an error value. And the earlier implementation was not actually doing that correctly.

Yes, we could make that part of the PR description.

@aranjans @arvindbr8
fix: updated comment
fc2054e 
Co-authored-by: Arvind Bright <arvind.bright100@gmail.com>
@aranjans aranjans assigned easwars and unassigned aranjans Aug 30, 2024
@arvindbr8 arvindbr8 changed the title xds/balancer/clusterimpl: Replace run goroutine with grpcsync.CallbackSerializer xds/clusterimpl: update UpdateClientConnState to handle updates synchronously Aug 30, 2024
@arvindbr8 arvindbr8 merged commit 00514a7 into grpc:master Aug 30, 2024
14 checks passed
@tbg tbg mentioned this pull request Nov 27, 2024
Draft
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arvindbr8 arvindbr8 arvindbr8 approved these changes

@easwars easwars easwars approved these changes

Assignees

@easwars easwars

Labels
Type: Internal Cleanup Refactors, etc
Projects
None yet
Milestone
1.67 Release
Development

Successfully merging this pull request may close these issues.
3 participants
@aranjans @easwars @arvindbr8