Skip to content

xds: remove temporary environment variable for least request #8248

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

atollena
Copy link
Collaborator

@atollena atollena commented Apr 14, 2025

Fixes #8228.

RELEASE NOTES: Remove the GRPC_EXPERIMENTAL_ENABLE_LEAST_REQUEST environment variable that allows disabling the least request balancer with xDS. Least request was made available by default with xDS in v1.72.0.

@atollena atollena added the Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. label Apr 14, 2025
@atollena atollena added this to the 1.73 Release milestone Apr 14, 2025
@atollena atollena added the Type: Internal Cleanup Refactors, etc label Apr 14, 2025
Copy link

codecov bot commented Apr 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.13%. Comparing base (718c4d8) to head (3b1137d).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8248      +/-   ##
==========================================
- Coverage   82.15%   82.13%   -0.03%     
==========================================
  Files         412      412              
  Lines       40477    40474       -3     
==========================================
- Hits        33253    33242      -11     
- Misses       5864     5866       +2     
- Partials     1360     1366       +6     
Files with missing lines Coverage Δ
internal/envconfig/envconfig.go 100.00% <ø> (ø)
...nal/xdsclient/xdslbregistry/converter/converter.go 70.00% <ø> (-0.63%) ⬇️
...ds/internal/xdsclient/xdsresource/unmarshal_cds.go 86.72% <ø> (+0.64%) ⬆️

... 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 atollena changed the title xds: Remove temporary environment variable for least request. xds: Remove temporary environment variable for least request Apr 15, 2025
@atollena
Copy link
Collaborator Author

As discussed I'll keep this PR for later and work on a change that just flips the environment variable default to true.

@atollena atollena changed the title xds: Remove temporary environment variable for least request xds: remove temporary environment variable for least request Apr 16, 2025
@atollena atollena closed this Apr 16, 2025
@atollena atollena reopened this Apr 17, 2025
@arjan-bal
Copy link
Contributor

@dfawley is it sufficient for one minor version (1.72) to have the flag defaulted to true before removing the env var completely? If yes, we can review and merge this.

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.

In this particular case, I think it's fine. I am not really concerned that registering this LB policy by default is going to cause problems. For other things that might be more risky, because we know our users don't always upgrade in a timely fashion, I think it's better to wait at least 2-3 releases before removing the flag.

@dfawley
Copy link
Member

dfawley commented May 5, 2025

@atollena can you please take a look at the merge conflicts? Thanks!

@dfawley dfawley assigned atollena and arjan-bal and unassigned dfawley May 5, 2025
@dfawley
Copy link
Member

dfawley commented May 5, 2025

@arjan-bal please do a second review & approve.

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: Internal Cleanup Refactors, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make least request available by default (remove GRPC_EXPERIMENTAL_ENABLE_LEAST_REQUEST)
3 participants