Skip to content

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

@codecov
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:

@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!

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.

}

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
@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