Skip to content

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

Merged
zasweq merged 5 commits into
grpc:masterfrom
zasweq:switch-wrr-to-pf
Nov 23, 2024
Merged

balancer/weightedroundrobin: Switch Weighted Round Robin to use pick first instead of SubConns#7826
zasweq merged 5 commits into
grpc:masterfrom
zasweq:switch-wrr-to-pf

Conversation

@zasweq

@zasweq zasweq commented Nov 11, 2024

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Nov 11, 2024

Copy link
Copy Markdown

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:

Comment thread balancer/endpointsharding/endpointsharding.go Outdated
Comment thread balancer/weightedroundrobin/balancer.go Outdated
Comment thread balancer/weightedroundrobin/balancer.go Outdated
Comment thread balancer/weightedroundrobin/balancer.go Outdated
Comment thread balancer/weightedroundrobin/balancer.go
Comment thread balancer/weightedroundrobin/balancer.go
Comment thread balancer/weightedroundrobin/balancer.go
Comment thread balancer/weightedroundrobin/balancer_test.go Outdated
Comment thread balancer/weightedroundrobin/balancer_test.go Outdated
Comment thread balancer/weightedroundrobin/balancer_test.go Outdated
@dfawley dfawley assigned zasweq and unassigned dfawley Nov 11, 2024
@zasweq zasweq assigned dfawley and unassigned zasweq Nov 18, 2024
@dfawley

dfawley commented Nov 19, 2024

Copy link
Copy Markdown
Member

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

zasweq commented Nov 21, 2024

Copy link
Copy Markdown
Contributor Author

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

@dfawley dfawley left a comment

Copy link
Copy Markdown
Member

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!

Comment thread balancer/weightedroundrobin/balancer.go Outdated
return nil, err
}
b.scToWeight[sc] = ewi.(*endpointWeight)
return sc, err

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, done.

Comment thread balancer/weightedroundrobin/balancer.go Outdated
}

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

Copy link
Copy Markdown
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
Copy Markdown
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
1 check passed
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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