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

balancer/weightedroundrobin: Switch Weighted Round Robin to use pick first instead of SubConns #7826

Merged
merged 5 commits into from
Nov 23, 2024

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Nov 11, 2024

This PR switches weighted round robin to defer to pick first instead of creating and handling SubConns itself. This is part of DualStack, and makes this a petiole.

RELEASE NOTES:

  • balancer/weightedroundrobin: Switch Weighted Round Robin to use pick first instead of SubConns

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 85.38813% with 32 lines in your changes missing coverage. Please review.

Project coverage is 81.82%. Comparing base (d2c1aae) to head (d026beb).
Report is 25 commits behind head on master.

Files with missing lines Patch % Lines
balancer/weightedroundrobin/balancer.go 85.92% 22 Missing and 7 partials ⚠️
balancer/endpointsharding/endpointsharding.go 66.66% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7826      +/-   ##
==========================================
- Coverage   81.84%   81.82%   -0.03%     
==========================================
  Files         373      375       +2     
  Lines       37845    37984     +139     
==========================================
+ Hits        30976    31082     +106     
- Misses       5578     5590      +12     
- Partials     1291     1312      +21     
Files with missing lines Coverage Δ
balancer/weightedroundrobin/scheduler.go 96.20% <100.00%> (-3.80%) ⬇️
balancer/endpointsharding/endpointsharding.go 79.65% <66.66%> (+16.62%) ⬆️
balancer/weightedroundrobin/balancer.go 83.47% <85.92%> (+4.01%) ⬆️

... and 48 files with indirect coverage changes

---- 🚨 Try these New Features:

balancer/endpointsharding/endpointsharding.go Outdated Show resolved Hide resolved
balancer/weightedroundrobin/balancer.go Outdated Show resolved Hide resolved
balancer/weightedroundrobin/balancer.go Outdated Show resolved Hide resolved
balancer/weightedroundrobin/balancer.go Outdated Show resolved Hide resolved
balancer/weightedroundrobin/balancer.go Show resolved Hide resolved
balancer/weightedroundrobin/balancer.go Outdated Show resolved Hide resolved
balancer/weightedroundrobin/balancer.go Show resolved Hide resolved
balancer/weightedroundrobin/balancer_test.go Outdated Show resolved Hide resolved
balancer/weightedroundrobin/balancer_test.go Outdated Show resolved Hide resolved
balancer/weightedroundrobin/balancer_test.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned zasweq and unassigned dfawley Nov 11, 2024
@zasweq zasweq assigned dfawley and unassigned zasweq Nov 18, 2024
@dfawley
Copy link
Member

dfawley commented Nov 19, 2024

LGTM modulo the one issue mentioned above.

@dfawley dfawley assigned zasweq and unassigned dfawley Nov 19, 2024
@zasweq zasweq assigned dfawley and unassigned zasweq Nov 21, 2024
@zasweq
Copy link
Contributor Author

zasweq commented Nov 21, 2024

Got to the two points we discussed in our 1:1.

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Two minor requests, otherwise LGTM. Thanks!

if err != nil {
return nil, err
}
b.scToWeight[sc] = ewi.(*endpointWeight)
return sc, err
Copy link
Member

Choose a reason for hiding this comment

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

nil explicitly, please, since you asserted err == nil already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, done.

@@ -463,7 +467,7 @@ func (p *picker) regenerateScheduler() {
atomic.StorePointer(&p.scheduler, unsafe.Pointer(&s))
}

func (p *picker) start(ctx context.Context) {
func (p *picker) start(done *grpcsync.Event) {
Copy link
Member

Choose a reason for hiding this comment

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

stopPicker so the name is consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dfawley dfawley assigned zasweq and unassigned dfawley Nov 22, 2024
@zasweq zasweq merged commit 13d5a16 into grpc:master Nov 23, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Behavior Change Behavior changes not categorized as bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants