Skip to content

Conversation

atollena
Copy link
Collaborator

@atollena atollena commented May 16, 2025

There was a leftover embedded balancer in least request that was never set in the code when it got refactored to pick first as leaf. As a result, method calls forwarded to the embedded balancer would panic.

Remove the embedded balance and properly forward the missing methods.

Fixes #8332

RELEASE NOTES:

  • balancer/least_request: fix panic on resolver errors when using least_request.

There was a leftover embedded balancer in least request that was never set in
the code when it got refactored to pick first as leaf. As a result, method calls
forwarded to the embedded balancer would panic.

Remove the embedded balance and properly forward the missing methods.

Fixes grpc#8332.
@atollena atollena requested a review from arjan-bal May 16, 2025 15:10
@atollena atollena added the Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. label May 16, 2025
@atollena atollena added this to the 1.73 Release milestone May 16, 2025
Copy link

codecov bot commented May 16, 2025

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes missing coverage. Please review.

Project coverage is 82.32%. Comparing base (f2d3e11) to head (e57a5de).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
balancer/leastrequest/leastrequest.go 44.44% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8333      +/-   ##
==========================================
- Coverage   82.41%   82.32%   -0.10%     
==========================================
  Files         419      419              
  Lines       42025    42034       +9     
==========================================
- Hits        34637    34604      -33     
- Misses       5944     5974      +30     
- Partials     1444     1456      +12     
Files with missing lines Coverage Δ
balancer/leastrequest/leastrequest.go 85.24% <44.44%> (-3.25%) ⬇️

... and 21 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@atollena
Copy link
Collaborator Author

Looking at the coverage report, I'm unclear on why the test doesn't cover the added implementation... Will look into it. Good to review otherwise.

@arjan-bal arjan-bal assigned atollena and unassigned arjan-bal May 16, 2025
@atollena atollena assigned arjan-bal and unassigned atollena May 19, 2025
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.

LGTM, thank you for the fix.

@arjan-bal arjan-bal changed the title Fix panic in least_request balancer least_request : Fix panic while handling resolver errors May 19, 2025
@arjan-bal arjan-bal changed the title least_request : Fix panic while handling resolver errors balancer/least_request : Fix panic while handling resolver errors May 19, 2025
@arjan-bal arjan-bal merged commit 0c24af1 into grpc:master May 19, 2025
23 of 24 checks passed
arjan-bal pushed a commit to arjan-bal/grpc-go that referenced this pull request May 19, 2025
arjan-bal pushed a commit to arjan-bal/grpc-go that referenced this pull request May 19, 2025
arjan-bal pushed a commit to arjan-bal/grpc-go that referenced this pull request May 19, 2025
@arjan-bal arjan-bal modified the milestones: 1.73 Release, 1.71 Release May 19, 2025
arjan-bal added a commit that referenced this pull request May 19, 2025
hugehoo pushed a commit to hugehoo/grpc-go that referenced this pull request May 20, 2025
hugehoo pushed a commit to hugehoo/grpc-go that referenced this pull request May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Least request causes panic on ReporterError starting in 1.71.0
2 participants