balancer/randomsubsetting: Implementation of the random_subsetting LB policy #8650

marek-szews · 2025-10-14T11:44:18Z

Implements [gRFC A68]https://github.com/grpc/proposal/blob/master/A68-random-subsetting.md.

Note that this PR only implements the LB policy and does not implement the xDS integration specified here: https://github.com/grpc/proposal/blob/master/A68-random-subsetting.md#xds-integration

RELEASE NOTES:

balancer/randomsubsetting: Implementation of the random_subsetting LB policy

Signed-off-by: marek-szews <[email protected]>

linux-foundation-easycla · 2025-10-14T11:44:24Z

The committers listed above are authorized under a signed CLA.

✅ login: easwars / name: Easwar Swaminathan (74cfecd, c4f8448, ddc8d09, f577218)
✅ login: marek-szews / name: Marek Szews (3c3f093, 7f43d9e, db49490, e34e0b9, e8ff4a1)

easwars · 2025-10-15T19:39:09Z

@marek-szews : Could you please get the tests to pass. Thanks.

balancer/randomsubsetting/randomsubsetting.go

github-actions · 2025-10-23T08:49:39Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

dfawley · 2025-10-29T16:04:09Z

Hi @marek-szews - I think we're waiting on you to make some updates so we can continue the review. We can discuss Friday but I wanted to ping this in part to prevent the stale bot from closing it.

marek-szews

Rework done, tests passed

balancer/randomsubsetting/randomsubsetting.go

easwars · 2025-11-13T20:58:57Z

Rework done, tests passed

Tests are still failing. Please read this document for guidelines: https://github.com/grpc/grpc-go/blob/master/CONTRIBUTING.md

TL;dr

All tests need to be passing before your change can be merged. We recommend you run tests locally before creating your PR to catch breakages early on:

./scripts/vet.sh to catch vet errors.
go test -cpu 1,4 -timeout 7m ./... to run the tests.
go test -race -cpu 1,4 -timeout 7m ./... to run tests in race mode

… fields

… a prefix logger

easwars · 2025-11-13T21:37:21Z

balancer/randomsubsetting/randomsubsetting.go

+	}
+
+	if b.cfg == nil || b.cfg.ChildPolicy.Name != lbCfg.ChildPolicy.Name {
+


Please remove this newline.

I'm talking about the newline after the if and before the call to SwitchTo.

balancer/randomsubsetting/randomsubsetting.go

balancer/randomsubsetting/randomsubsetting_test.go

easwars · 2025-11-13T22:00:02Z

balancer/randomsubsetting/randomsubsetting_test.go

+		maxDiff    int
+	}{
+		{
+			name:       "backends could be evenly distributed between small number of clients",


Please see here for guidelines on how best to name subtests: https://google.github.io/styleguide/go/decisions#subtest-names

TL;dr
Think of subtest names more like a function identifier than a prose description.

The test runner replaces spaces with underscores, and escapes non-printing characters. To ensure accurate correlation between test logs and source code, it is recommended to avoid using these characters in subtest names.

balancer/randomsubsetting/randomsubsetting.go

github-actions · 2025-11-27T08:50:15Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

codecov · 2025-12-04T19:24:39Z

Codecov Report

❌ Patch coverage is 80.64516% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.19%. Comparing base (8110884) to head (db49490).
⚠️ Report is 51 commits behind head on master.

Files with missing lines	Patch %	Lines
balancer/randomsubsetting/randomsubsetting.go	80.64%	7 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8650      +/-   ##
==========================================
+ Coverage   79.45%   83.19%   +3.74%     
==========================================
  Files         415      419       +4     
  Lines       41339    32429    -8910     
==========================================
- Hits        32844    26978    -5866     
+ Misses       6621     4062    -2559     
+ Partials     1874     1389     -485

Files with missing lines	Coverage Δ
balancer/randomsubsetting/randomsubsetting.go	`80.64% <80.64%> (ø)`

... and 373 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

easwars · 2025-12-05T06:14:33Z

balancer/randomsubsetting/randomsubsetting.go

+	}
+
+	if b.cfg == nil || b.cfg.ChildPolicy.Name != lbCfg.ChildPolicy.Name {
+


I'm talking about the newline after the if and before the call to SwitchTo.

easwars · 2025-12-05T06:15:16Z

balancer/randomsubsetting/randomsubsetting.go

+func (b *subsettingBalancer) UpdateClientConnState(s balancer.ClientConnState) error {
+	lbCfg, ok := s.BalancerConfig.(*lbConfig)
+	if !ok {
+		b.logger.Errorf("Received config with unexpected type %T: %v", s.BalancerConfig, s.BalancerConfig)


This should probably be logged at WARNING level instead of error. Please see https://github.com/grpc/grpc-go/blob/master/Documentation/log_levels.md for guidance on what logging levels to use.

easwars · 2025-12-05T06:19:55Z

balancer/randomsubsetting/randomsubsetting.go

+func (b *subsettingBalancer) prepareChildResolverState(s balancer.ClientConnState) resolver.State {
+	subsetSize := b.cfg.SubsetSize
+	endpoints := s.ResolverState.Endpoints
+	if len(endpoints) <= int(subsetSize) || subsetSize < 2 {


The subsetting algorithm described in the gRFC does not specify any special considerations for subset sizes less than two. Please see: https://github.com/grpc/proposal/blob/master/A68-random-subsetting.md#subsetting-algorithm

So, please remove this condition subsetSize < 2. If the user wants a subset size of 1, they should get just that.

easwars · 2025-12-05T06:22:08Z

balancer/randomsubsetting/randomsubsetting.go

+
+// implements the subsetting algorithm,
+// as described in A68: https://github.com/grpc/proposal/blob/master/A68-random-subsetting.md
+func (b *subsettingBalancer) prepareChildResolverState(s balancer.ClientConnState) resolver.State {


Is there any reason for this method to accept a balancer.ClientConnState and return a resolver.State. Why not accept a []resolver.Endpoint and return a []resolver.Endpoint? and let the caller deal with creating the resolver.State to be passed to the child.

Also, when we do that, we should rename this method appropriately.

Yes, it's a good idea to do this, especially since the interface of this function has changed completely (in/out).
Done

easwars · 2025-12-05T06:22:49Z

balancer/randomsubsetting/randomsubsetting.go

+}
+
+// implements the subsetting algorithm,
+// as described in A68: https://github.com/grpc/proposal/blob/master/A68-random-subsetting.md


If adding a link, please add it to the specific sub-section in the gRFC that talks about the subsetting algorithm or just don't add one.

easwars · 2025-12-05T06:34:51Z

balancer/randomsubsetting/randomsubsetting_test.go

+ *
+ */
+
+// Package e2e_test contains e2e test cases for the Subsetting LB Policy.


This comment is not valid anymore.

easwars · 2025-12-05T06:35:56Z

balancer/randomsubsetting/randomsubsetting_test.go

+		wantErr string
+	}{
+		{
+			name:    "invalid-json",


Nit: s/invalid-json/invalid_json/

See: https://google.github.io/styleguide/go/decisions#subtest-names

easwars · 2025-12-05T06:38:58Z

balancer/randomsubsetting/randomsubsetting_test.go

+		{
+			name:    "invalid-json",
+			input:   "{{invalidjson{{",
+			wantErr: "invalid character",


This has now become a change detector test for the json package. Please change the error string returned when unmarshal fails so that you can look for that here instead. So, maybe something like:

if err := json.Unmarshal(s, lbCfg); err != nil { return nil, fmt.Errorf("randomsubsetting: json.Unmarshal failed for configuration: %s with error: %v", string(s), err) }

Then, your wantErr here could be "json.Unmarshal failed for configuration"

easwars · 2025-12-05T06:41:40Z

balancer/randomsubsetting/randomsubsetting_test.go

+			if diff := cmp.Diff(gotCfg, test.wantCfg); diff != "" {
+				t.Fatalf("ParseConfig(%v) got unexpected output, diff (-got +want): %v", test.input, diff)
+			}


I think the order of arguments passed to cmp.Diff and in the error message is inverted. See here for examples: https://google.github.io/styleguide/go/decisions#equality-comparison-and-diffs

easwars · 2025-12-05T06:47:09Z

balancer/randomsubsetting/randomsubsetting_test.go

+	}
+}
+
+func (s) TestSubsettingE2E(t *testing.T) {


The only real piece of functionality provided by this LB policy is the subsetting algorithm. If we refactor it the way I recommended in another comment, then we could simply have unit tests for that method to ensure that subsetting behavior is as expected.

And then we could have a functional test where we:

create an instance of the LB policy,

call its UpdateClientConnState method to pass it some configuration with a child policy that is controlled by the test. See here for how to define a balancer that is completely under the control of the test: https://github.com/grpc/grpc-go/blob/master/internal/balancer/stub/stub.go

Verify that the child policy gets the expected resolver.State

There is very little value is having an E2E style for this LB policy.

Implementation of A68 random_subsetting LB policy.

7f43d9e

Signed-off-by: marek-szews <[email protected]>

Change a coments

e8ff4a1

easwars self-assigned this Oct 15, 2025

easwars self-requested a review October 15, 2025 19:17

easwars assigned marek-szews and unassigned easwars Oct 15, 2025

easwars added the Status: Requires Reporter Clarification label Oct 15, 2025

easwars reviewed Oct 15, 2025

View reviewed changes

github-actions bot added stale and removed stale labels Oct 23, 2025

arjan-bal added this to the 1.78 Release milestone Oct 30, 2025

rework for A68

3c3f093

marek-szews commented Oct 31, 2025

View reviewed changes

dfawley assigned easwars and unassigned marek-szews Oct 31, 2025

dfawley removed the Status: Requires Reporter Clarification label Oct 31, 2025

easwars changed the title ~~Implementation of A68 random_subsetting LB policy.~~ balancer/randomsubsetting: Implementation of the LB policy Nov 13, 2025

easwars changed the title ~~balancer/randomsubsetting: Implementation of the LB policy~~ balancer/randomsubsetting: Implementation of the random_subsetting LB policy Nov 13, 2025

easwars assigned marek-szews and unassigned easwars Nov 13, 2025

embed the gracefulswitch.Balancer in subsettingBalancer

c4f8448

easwars added 3 commits November 13, 2025 21:07

use camelCase instead of snake_case in json annotations for LB config…

f577218

… fields

do not use the package name prefix in log messages, as we already use…

ddc8d09

… a prefix logger

improvements to ParseConfig and its tests

74cfecd

easwars reviewed Nov 13, 2025

View reviewed changes

eshitachandwani added the Status: Requires Reporter Clarification label Nov 21, 2025

github-actions bot added the stale label Nov 27, 2025

github-actions bot closed this Dec 4, 2025

Rework round 2th

e34e0b9

dfawley reopened this Dec 4, 2025

dfawley removed Status: Requires Reporter Clarification stale labels Dec 4, 2025

dfawley assigned easwars and unassigned marek-szews Dec 4, 2025

easwars reviewed Dec 5, 2025

View reviewed changes

easwars assigned marek-szews and unassigned easwars Dec 5, 2025

easwars added the Status: Requires Reporter Clarification label Dec 5, 2025

Rework round 3th

db49490

dfawley assigned easwars and unassigned marek-szews Dec 5, 2025

dfawley removed the Status: Requires Reporter Clarification label Dec 5, 2025

		}

		if b.cfg == nil \|\| b.cfg.ChildPolicy.Name != lbCfg.ChildPolicy.Name {

balancer/randomsubsetting: Implementation of the random_subsetting LB policy #8650

Are you sure you want to change the base?

balancer/randomsubsetting: Implementation of the random_subsetting LB policy #8650

Uh oh!

Conversation

marek-szews commented Oct 14, 2025 • edited by easwars Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

easwars commented Oct 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 23, 2025

Uh oh!

dfawley commented Oct 29, 2025

Uh oh!

marek-szews left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

easwars commented Nov 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Nov 27, 2025

Uh oh!

codecov bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marek-szews commented Oct 14, 2025 •

edited by easwars

Loading

linux-foundation-easycla bot commented Oct 14, 2025 •

edited

Loading

codecov bot commented Dec 4, 2025 •

edited

Loading