Implementation of A68 random_subsetting LB policy. #8650

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

Introduce a new LB policy, random_subsetting. This policy selects a subset of addresses and passes them to the child LB policy to correct the resulting server-side load balancing.

Signed-off-by: marek-szews <szews@google.com>

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

The committers listed above are authorized under a signed CLA.

✅ login: marek-szews / name: Marek Szews (3c3f093, 7f43d9e, e8ff4a1)

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

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

easwars · 2025-10-15T19:48:40Z

balancer/randomsubsetting/randomsubsetting.go

+	SubsetSize uint64 `json:"subset_size,omitempty"`
+
+	ChildPolicy *iserviceconfig.BalancerConfig `json:"child_policy,omitempty"`


We generally use camelCase for the json annotations since the protobuf library uses that when marshalling protobuf messages to JSON. So, please change these to use camelCase. Thanks.

When I have changed the name of variable to lower case I've got:
--- FAIL: Test (0.00s)
--- FAIL: Test/ParseConfig (0.00s)
--- FAIL: Test/ParseConfig/happy-case-default (0.00s)
panic: cannot handle unexported field at {*randomsubsetting.LBConfig}.subsetSize:
"google.golang.org/grpc/balancer/randomsubsetting".LBConfig
consider using cmpopts.EquateComparable to compare comparable Go types [recovered]
panic: cannot handle unexported field at {*randomsubsetting.LBConfig}.subsetSize:
"google.golang.org/grpc/balancer/randomsubsetting".LBConfig
consider using cmpopts.EquateComparable to compare comparable Go types

easwars · 2025-10-15T19:49:00Z

balancer/randomsubsetting/randomsubsetting.go

+	return b
+}
+
+// LBConfig is the config for the outlier detection balancer.


This comment needs to be updated.

easwars · 2025-10-15T19:49:40Z

balancer/randomsubsetting/randomsubsetting.go

+}
+
+// LBConfig is the config for the outlier detection balancer.
+type LBConfig struct {


Is there a reason for this type to be exported. I don't see a reason here. So, if there is no reason, please unexport it. The fields can remain exported since you might want that for JSON marshal/unmarshal.

Yes, we need to use this type in the test function.

easwars · 2025-10-15T19:50:20Z

balancer/randomsubsetting/randomsubsetting.go

+		// Default top layer values.
+		SubsetSize: 10,


Where is this default value specified?

In the previous proposal of config I have found somthing like this (PROP-423):
message RandomSubsettingLbConfig {
// subset_size indicates how many backends every client will be connected to.
// Default is 20.
google.protobuf.UInt32Value subset_size = 1;
But I saw in the most current version (PROTO-157) that this information has been removed. My suggestion is to set subsetSize to 2 by default, because 1 should suggest to use pick_first instead.

easwars · 2025-10-15T19:52:14Z

balancer/randomsubsetting/randomsubsetting.go

+	}
+
+	// if someonw needs subsetSize == 1, he should use pick_first instead
+	if lbCfg.SubsetSize < 2 {


The gRFC only says the following:

// The value is required and must be greater than 0.

Is the condition that the size be at least 2 specified somewhere else?

See the discussion at line 86

easwars · 2025-10-15T20:05:08Z

balancer/randomsubsetting/randomsubsetting.go

+		return addressesSet[i].hash < addressesSet[j].hash
+	})
+
+	b.logger.Infof("resulting subset: %v", addressesSet[:b.cfg.SubsetSize])


Please guard this log line with a verbosity check of 2.

easwars · 2025-10-15T20:06:15Z

balancer/randomsubsetting/randomsubsetting.go

+
+// implements the subsetting algorithm, as described in A68: https://github.com/grpc/proposal/pull/423
+func (b *subsettingBalancer) prepareChildResolverState(s resolver.State) resolver.State {
+	addresses := s.Addresses


We should be using the Endpoints field in resolver.State for everything here (except for computing the hash where we use the first address inside of the endpoint).

easwars · 2025-10-15T20:06:43Z

balancer/randomsubsetting/randomsubsetting.go

+	// Convert back to resolver.addresses
+	addressesSubset := make([]resolver.Address, b.cfg.SubsetSize)
+	for _, eh := range addressesSet[:b.cfg.SubsetSize] {
+		addressesSubset = append(addressesSubset, eh.addr)
+	}
+
+	return resolver.State{
+		Addresses:     addressesSubset,
+		ServiceConfig: s.ServiceConfig,
+		Attributes:    s.Attributes,
+	}


All of this should deal with endpoints and not addresses.

easwars · 2025-10-15T20:08:29Z

balancer/randomsubsetting/randomsubsetting.go

+func (b *subsettingBalancer) ResolverError(err error) {
+	b.child.ResolverError(err)
+}
+
+func (b *subsettingBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.SubConnState) {
+	b.child.UpdateSubConnState(sc, state)
+}
+
+func (b *subsettingBalancer) Close() {
+	b.child.Close()
+}
+
+func (b *subsettingBalancer) ExitIdle() {
+	b.child.ExitIdle()
+}


Could we embed the gracefulswitch.Balancer in the subsettingBalancer, and thereby get rid of all these forwarding methods?

How we can do this ?

easwars · 2025-10-15T20:09:42Z

balancer/randomsubsetting/randomsubsetting.go

+	ChildPolicy *iserviceconfig.BalancerConfig `json:"child_policy,omitempty"`
+}
+
+func (bb) ParseConfig(s json.RawMessage) (serviceconfig.LoadBalancingConfig, error) {


We need to add unit tests for this method. Thanks.

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

marek-szews · 2025-10-17T08:39:45Z

balancer/randomsubsetting/randomsubsetting.go

+	SubsetSize uint64 `json:"subset_size,omitempty"`
+
+	ChildPolicy *iserviceconfig.BalancerConfig `json:"child_policy,omitempty"`


When I have changed the name of variable to lower case I've got:
--- FAIL: Test (0.00s)
--- FAIL: Test/ParseConfig (0.00s)
--- FAIL: Test/ParseConfig/happy-case-default (0.00s)
panic: cannot handle unexported field at {*randomsubsetting.LBConfig}.subsetSize:
"google.golang.org/grpc/balancer/randomsubsetting".LBConfig
consider using cmpopts.EquateComparable to compare comparable Go types [recovered]
panic: cannot handle unexported field at {*randomsubsetting.LBConfig}.subsetSize:
"google.golang.org/grpc/balancer/randomsubsetting".LBConfig
consider using cmpopts.EquateComparable to compare comparable Go types

marek-szews · 2025-10-17T14:02:18Z

balancer/randomsubsetting/randomsubsetting.go

+		// Default top layer values.
+		SubsetSize: 10,


In the previous proposal of config I have found somthing like this (PROP-423):
message RandomSubsettingLbConfig {
// subset_size indicates how many backends every client will be connected to.
// Default is 20.
google.protobuf.UInt32Value subset_size = 1;
But I saw in the most current version (PROTO-157) that this information has been removed. My suggestion is to set subsetSize to 2 by default, because 1 should suggest to use pick_first instead.

marek-szews · 2025-10-24T09:06:33Z

balancer/randomsubsetting/randomsubsetting.go

+	ChildPolicy *iserviceconfig.BalancerConfig `json:"child_policy,omitempty"`
+}
+
+func (bb) ParseConfig(s json.RawMessage) (serviceconfig.LoadBalancingConfig, error) {


marek-szews · 2025-10-24T11:33:36Z

balancer/randomsubsetting/randomsubsetting.go

+	}
+
+	if err := json.Unmarshal(s, lbCfg); err != nil { // Validates child config if present as well.
+		return nil, fmt.Errorf("subsetting: unable to unmarshal LBconfig: %s, error: %v", string(s), err)


marek-szews · 2025-10-28T10:10:11Z

balancer/randomsubsetting/randomsubsetting.go

+		SubsetSize: 10,
+	}
+
+	if err := json.Unmarshal(s, lbCfg); err != nil { // Validates child config if present as well.


marek-szews · 2025-10-31T09:05:31Z

balancer/randomsubsetting/randomsubsetting.go

+
+// implements the subsetting algorithm, as described in A68: https://github.com/grpc/proposal/pull/423
+func (b *subsettingBalancer) prepareChildResolverState(s resolver.State) resolver.State {
+	addresses := s.Addresses


marek-szews · 2025-10-31T09:05:44Z

balancer/randomsubsetting/randomsubsetting.go

+	addressesSet := make([]AddressWithHash, backendCount)
+	// calculate hash for each endpoint
+	for i, endpoint := range addresses {
+


marek-szews · 2025-10-31T12:30:16Z

balancer/randomsubsetting/randomsubsetting.go

+		return addressesSet[i].hash < addressesSet[j].hash
+	})
+
+	b.logger.Infof("resulting subset: %v", addressesSet[:b.cfg.SubsetSize])


marek-szews · 2025-10-31T12:30:31Z

balancer/randomsubsetting/randomsubsetting.go

+	// Convert back to resolver.addresses
+	addressesSubset := make([]resolver.Address, b.cfg.SubsetSize)
+	for _, eh := range addressesSet[:b.cfg.SubsetSize] {
+		addressesSubset = append(addressesSubset, eh.addr)
+	}
+
+	return resolver.State{
+		Addresses:     addressesSubset,
+		ServiceConfig: s.ServiceConfig,
+		Attributes:    s.Attributes,
+	}


marek-szews · 2025-10-31T12:32:07Z

balancer/randomsubsetting/randomsubsetting.go

+func (b *subsettingBalancer) ResolverError(err error) {
+	b.child.ResolverError(err)
+}
+
+func (b *subsettingBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.SubConnState) {
+	b.child.UpdateSubConnState(sc, state)
+}
+
+func (b *subsettingBalancer) Close() {
+	b.child.Close()
+}
+
+func (b *subsettingBalancer) ExitIdle() {
+	b.child.ExitIdle()
+}


How we can do this ?

Implementation of A68 random_subsetting LB policy.

7f43d9e

Signed-off-by: marek-szews <szews@google.com>

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

		SubsetSize uint64 `json:"subset_size,omitempty"`

		ChildPolicy *iserviceconfig.BalancerConfig `json:"child_policy,omitempty"`

Implementation of A68 random_subsetting LB policy. #8650

Are you sure you want to change the base?

Implementation of A68 random_subsetting LB policy. #8650

Uh oh!

Conversation

marek-szews commented Oct 14, 2025

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!

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 Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

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!

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!

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

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

edited

Loading

marek-szews Oct 17, 2025 •

edited

Loading