balancer: rewrite the consistent hashring balancer to avoid recomputations #1310

ecordell · 2023-05-11T14:56:55Z

The previous implementation was recomputing the hashring any time a subconnection moved from ready->idle or back, which happened frequently.

The new implementation includes idle and connecting subconns in the ring, and triggers a connection if one is selected. It also adds/removes from a long-lived ring instead of recomputing a ring from scratch each time.

ReplicationFactor and Spread can now be passed in as part of the service config instead of registered globally with the balancer

josephschorr · 2023-05-17T23:53:44Z

pkg/balancer/hashring.go

+	ReplicationFactor: 100,
+	Spread:            1,
+}
+var DefaultBalancerServiceConfigJSON = defaultBalancerServiceConfig.MustToServiceConfigJSON()


doc comments on exported stuff

can we find a lint rule for this? I feel like we started it because of lint rules but no linter complains anymore

Added the config for golangci-lint

josephschorr · 2023-05-17T23:54:55Z

pkg/balancer/hashring.go

+	// Initialize picker to a picker that always returns
+	// ErrNoSubConnAvailable, because when state of a SubConn changes, we
+	// may call UpdateState with this picker.
+	bal.picker = base.NewErrPicker(balancer.ErrNoSubConnAvailable)


why is this done this way vs just doing so in the struct above?

josephschorr · 2023-05-17T23:55:11Z

pkg/balancer/hashring.go

+	logger.Infof("parsed balancer config %s", js)
+
+	if lbCfg.ReplicationFactor == 0 {
+		lbCfg.ReplicationFactor = 100


can we have these defaults be in constants?

josephschorr · 2023-05-17T23:56:36Z

pkg/balancer/hashring.go

-	if len(info.ReadySCs) == 0 {
-		return base.NewErrPicker(balancer.ErrNoSubConnAvailable)
+func (b *ConsistentHashringBalancer) UpdateClientConnState(s balancer.ClientConnState) error {
+	if logger.V(2) {


do we do this kind of log level check anywhere else?

this is using the grpc logging framework, not logrus

josephschorr · 2023-05-17T23:56:58Z

pkg/balancer/hashring.go

-	logger.Infof("consistentHashringPicker: Build called with info: %v", info)
-	if len(info.ReadySCs) == 0 {
-		return base.NewErrPicker(balancer.ErrNoSubConnAvailable)
+func (b *ConsistentHashringBalancer) UpdateClientConnState(s balancer.ClientConnState) error {


can you add a few more comments describing the workflow here?

josephschorr · 2023-05-17T23:58:18Z

pkg/balancer/hashring.go

+	b.cc.UpdateState(balancer.State{ConnectivityState: b.state, Picker: b.picker})
+}
+
+// Close is a nop because base balancer doesn't have internal state to clean up,


josephschorr · 2023-05-17T23:59:16Z

pkg/balancer/hashring_test.go

+// Note: this is testing picker behavior and not the hashring
+// behavior itself, see `pkg/consistent` for tests of the hashring.
+func TestConsistentHashringPickerPick(t *testing.T) {
+	rnd := rand.New(rand.NewSource(1))


add a comment here that we are pinning the seed for below

josephschorr · 2023-05-17T23:59:41Z

pkg/balancer/hashring_test.go

+				t.Errorf("Pick() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {


t.True(reflect.DeepEqual(got, tt.want), "Pick() got ...")?

josephschorr · 2023-05-17T23:59:58Z

pkg/balancer/hashring_test.go

+		wantErr           bool
+	}{
+		{
+			name:              "sets rf and spread",


only one test case in a table-driven test?

added some more

josephschorr · 2023-05-18T00:00:34Z

pkg/balancer/hashring_test.go

+				Spread:            tt.spread,
+			}
+			got, err := c.ToServiceConfigJSON()
+			if (err != nil) != tt.wantErr {


I generally prefer to do

// ensure error return } // ensure non-error case

I can change it; I sometimes use goland's test scaffolding which writes assertions like this, and I don't always change them to more idiomatic testify

josephschorr · 2023-05-18T15:44:35Z

pkg/balancer/hashring.go

+// ParseConfig satisfies balancer.ConfigParser and is used to parse new
+// Service Config json. The results are stored on the builder so that
+// subsequently built Balancers use the config.
+func (b *ConsistentHashringBuilder) ParseConfig(js json.RawMessage) (serviceconfig.LoadBalancingConfig, error) {


seems weird that ParseConfig stores a value, but since that's what the interface requires, guess its okay

josephschorr · 2023-05-18T15:45:07Z

pkg/balancer/hashring.go

 	}
+	// Successful resolution; clear resolver error and ensure we return nil.


josephschorr · 2023-05-18T15:46:08Z

pkg/balancer/hashring.go

+	// any that have been removed since the last update are removed from the
+	// hashring.
+	addrsSet := resolver.NewAddressMap()
+	for _, a := range s.ResolverState.Addresses {


maybe use addr?

josephschorr · 2023-05-18T15:46:53Z

pkg/balancer/hashring.go

+		if _, ok := addrsSet.Get(a); !ok {
+			b.cc.RemoveSubConn(sc)
+			b.subConns.Delete(a)
+			// Keep the state of this sc in b.scStates until sc's state becomes Shutdown.


odd spacing here

josephschorr · 2023-05-18T15:47:20Z

pkg/balancer/hashring.go

+		return balancer.ErrBadResolverState
+	}
+
+	// if the overall connection sate is not in transient failure, we return


sate -> state

josephschorr · 2023-05-18T15:47:57Z

pkg/balancer/hashring.go

+// Pick satisfies balancer.Picker and returns a subconnection to use for a
+// request based on the request info. The value stored in CtxKey is hashed
+// into the hashring, and the resulting subconnection is used. Note that
+// there is no fallback behavior if the subconnection; this prevents the


if the subconnection... ?

josephschorr

LGTM

The previous implementation was recomputing the hashring any time a subconnection moved from ready->idle or back, which happened frequently. The new implementation includes idle and connecting subconns in the ring, and triggers a connection if one is selected. It also adds/removes from a long-lived ring instead of recomputing a ring from scratch each time. ReplicationFactor and Spread can now be passed in as part of the service config instead of registered globally with the balancer

github-actions bot added the area/CLI Affects the command line label May 11, 2023

ecordell force-pushed the idlepickerhashring branch 2 times, most recently from 47996e3 to 1ca1002 Compare May 11, 2023 18:32

ecordell changed the title ~~balancer: rewrite the consistent hashring balancer to avoid recomputates~~ balancer: rewrite the consistent hashring balancer to avoid recomputations May 11, 2023

ecordell force-pushed the idlepickerhashring branch from 1ca1002 to 53c6a19 Compare May 12, 2023 14:56

github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label May 12, 2023

ecordell force-pushed the idlepickerhashring branch from 53c6a19 to 23c2ed8 Compare May 12, 2023 15:01

ecordell marked this pull request as ready for review May 12, 2023 15:01

ecordell requested a review from a team May 12, 2023 15:01

ecordell force-pushed the idlepickerhashring branch 3 times, most recently from b79aac4 to 36d5e41 Compare May 15, 2023 14:29

josephschorr self-requested a review May 17, 2023 23:51

ecordell force-pushed the idlepickerhashring branch from 36d5e41 to 501590b Compare May 17, 2023 23:51

josephschorr reviewed May 18, 2023

View reviewed changes

ecordell force-pushed the idlepickerhashring branch 3 times, most recently from ef2f607 to 5bb867e Compare May 18, 2023 03:05

josephschorr reviewed May 18, 2023

View reviewed changes

ecordell force-pushed the idlepickerhashring branch from 5bb867e to 302ba2b Compare May 18, 2023 16:07

josephschorr approved these changes May 18, 2023

View reviewed changes

ecordell force-pushed the idlepickerhashring branch from 302ba2b to 89100db Compare May 18, 2023 16:14

ecordell enabled auto-merge May 18, 2023 16:14

ecordell force-pushed the idlepickerhashring branch from 89100db to 94145c0 Compare May 18, 2023 16:20

ecordell added this pull request to the merge queue May 18, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 18, 2023

ecordell added this pull request to the merge queue May 18, 2023

Merged via the queue into authzed:main with commit 0026188 May 18, 2023

ecordell deleted the idlepickerhashring branch May 18, 2023 17:10

github-actions bot locked and limited conversation to collaborators May 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

balancer: rewrite the consistent hashring balancer to avoid recomputations #1310

balancer: rewrite the consistent hashring balancer to avoid recomputations #1310

ecordell commented May 11, 2023

josephschorr May 17, 2023

ecordell May 18, 2023

ecordell May 18, 2023

ecordell May 18, 2023

josephschorr May 17, 2023

josephschorr May 17, 2023

josephschorr May 17, 2023

ecordell May 18, 2023

josephschorr May 17, 2023

josephschorr May 17, 2023

josephschorr May 17, 2023

josephschorr May 17, 2023

josephschorr May 17, 2023

ecordell May 18, 2023

josephschorr May 18, 2023

ecordell May 18, 2023

josephschorr May 18, 2023

josephschorr May 18, 2023

josephschorr May 18, 2023

josephschorr May 18, 2023

josephschorr May 18, 2023

josephschorr May 18, 2023

josephschorr left a comment

		}
		// Successful resolution; clear resolver error and ensure we return nil.

balancer: rewrite the consistent hashring balancer to avoid recomputations #1310

balancer: rewrite the consistent hashring balancer to avoid recomputations #1310

Conversation

ecordell commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr left a comment

Choose a reason for hiding this comment