balancer/resolver: add loadBalancingConfig and pre-parsing support #2732

dfawley · 2019-03-29T22:44:58Z

This is still subject to change; pushing now to get initial round of feedback.

This change is

menghanl · 2019-04-03T00:10:04Z

balancer/balancer.go

@@ -166,6 +170,14 @@ type Builder interface {
 	Name() string
 }

+// Parser parses service configs.
+type Parser interface {


ConfigParser?

(Or even, JsonConfigParser?)

menghanl · 2019-04-03T00:18:44Z

balancer_conn_wrappers.go

-			if s.Addresses[i].Type == resolver.GRPCLB {
-				copy(s.Addresses[i:], s.Addresses[i+1:])
-				s.Addresses = s.Addresses[:len(s.Addresses)-1]
+		for i := 0; i < len(s.ResolverState.Addresses); {


Nit: make a local variable? Is go smart enough to optimize out the dereference cost?

Or, rename the parameter to css, and then s := css.ResolverState.

Done (second one) - I'm actually modifying the state to remove those addresses, so making a local holding the slice won't update the contents of the ClientConnState properly (i.e. making it shorter).

menghanl · 2019-04-03T00:22:07Z

clientconn.go

-		if err != nil {
-			fmt.Println("error parsing config: ", err)
-			return err
+		if sc, ok := s.ServiceConfig.(ServiceConfig); ok {


Compare service config?
(Compare raw json? This doesn't work because of balancer configs, right?)

I think the only place where checking it would matter is when we reset the retry throttling tokens, and that should probably be checked independently in case parts of the config change but the retry config doesn't.

Otherwise, it's just reassigning a pointer. WDYT?

menghanl · 2019-04-03T00:40:46Z

service_config.go

 }

+type loadBalancingConfig map[string]json.RawMessage


This type is only used once. Inline?

Inlining makes it really complex. *[]map[string]json.RawMessage - I'd rather give it a name and then have it be easier to understand the field in the jsonSC.

menghanl · 2019-04-03T00:43:58Z

service_config.go

+				continue
+			}
+			sc.lbConfig = &lbConfig{name: name}
+			if parser, ok := builder.(balancer.Parser); ok {


warning if not OK

menghanl · 2019-04-03T00:48:51Z

serviceconfig/serviceconfig.go

+
+// Parse parses the JSON service config provided into an internal form or
+// returns an error if the config is invalid.
+func Parse(ServiceConfigJSON string) (Config, error) {


ParseJson?

I'd rather leave this unqualified. If we ever decide to support parsing another format in grpc-go, then the other formats can be namespaced.

dfawley

Everything is updated and should be good to review again. Now with xds and grpclb adoption of the V2 interfaces!

dfawley · 2019-05-21T22:25:29Z

balancer/balancer.go

@@ -166,6 +170,14 @@ type Builder interface {
 	Name() string
 }

+// Parser parses service configs.
+type Parser interface {


dfawley · 2019-05-21T22:29:01Z

balancer_conn_wrappers.go

-			if s.Addresses[i].Type == resolver.GRPCLB {
-				copy(s.Addresses[i:], s.Addresses[i+1:])
-				s.Addresses = s.Addresses[:len(s.Addresses)-1]
+		for i := 0; i < len(s.ResolverState.Addresses); {


Done (second one) - I'm actually modifying the state to remove those addresses, so making a local holding the slice won't update the contents of the ClientConnState properly (i.e. making it shorter).

dfawley · 2019-05-21T22:33:38Z

service_config.go

 }

+type loadBalancingConfig map[string]json.RawMessage


Inlining makes it really complex. *[]map[string]json.RawMessage - I'd rather give it a name and then have it be easier to understand the field in the jsonSC.

dfawley · 2019-05-21T22:36:02Z

service_config.go

+				continue
+			}
+			sc.lbConfig = &lbConfig{name: name}
+			if parser, ok := builder.(balancer.Parser); ok {


dfawley · 2019-05-21T22:37:21Z

serviceconfig/serviceconfig.go

+
+// Parse parses the JSON service config provided into an internal form or
+// returns an error if the config is invalid.
+func Parse(ServiceConfigJSON string) (Config, error) {


I'd rather leave this unqualified. If we ever decide to support parsing another format in grpc-go, then the other formats can be namespaced.

dfawley · 2019-05-24T17:44:55Z

clientconn.go

-		if err != nil {
-			fmt.Println("error parsing config: ", err)
-			return err
+		if sc, ok := s.ServiceConfig.(ServiceConfig); ok {


I think the only place where checking it would matter is when we reset the retry throttling tokens, and that should probably be checked independently in case parts of the config change but the retry config doesn't.

Otherwise, it's just reassigning a pointer. WDYT?

dfawley · 2019-05-24T19:51:58Z

balancer/xds/xds.go

-	case *resolver.State:
-		cfg := getBalancerConfig(u.ServiceConfig)
+	case *balancer.ClientConnState:
+		cfg, _ := u.BalancerConfig.(*xdsConfig)
 		if cfg == nil {
 			// service config parsing failed. should never happen. And this parsing will be removed, once


What should we do here? I think we need to leave this code for defensive programming, to avoid nil panics, but the comment could be updated, e.g. only say "should never happen"?

menghanl

Reviewed 7 of 16 files at r1, 16 of 16 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dfawley)

balancer/xds/xds.go, line 247 at r2 (raw file):

What should we do here? I think we need to leave this code for defensive programming, to avoid nil panics, but the comment could be updated, e.g. only say "should never happen"?

Yes, this needs to be done anyway in all balancers.

balancer/xds/xds.go, line 603 at r2 (raw file):

			}
			for _, lbcfg := range lbcfgs {
				if balancer.Get(lbcfg.Name) != nil {

Are you still planning to move this out from unmarshal?

dfawley

Reviewable status: 22 of 23 files reviewed, 2 unresolved discussions (waiting on @menghanl)

balancer/xds/xds.go, line 247 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

What should we do here? I think we need to leave this code for defensive programming, to avoid nil panics, but the comment could be updated, e.g. only say "should never happen"?

Yes, this needs to be done anyway in all balancers.

Done.

balancer/xds/xds.go, line 603 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Are you still planning to move this out from unmarshal?

Not in this PR. I want to avoid making unrelated changes for now to reduce risk on an otherwise-significant PR.

dfawley added the Type: Feature New features or improvements in behavior label Mar 29, 2019

dfawley assigned menghanl Mar 29, 2019

dfawley requested a review from menghanl March 29, 2019 22:44

menghanl reviewed Apr 3, 2019

View reviewed changes

dfawley assigned dfawley and unassigned menghanl Apr 22, 2019

menghanl mentioned this pull request May 16, 2019

Add a method for resolver to report error #2102

Closed

dfawley force-pushed the lbconfig branch from a73ba3e to 11472c4 Compare May 24, 2019 17:57

dfawley added 2 commits May 24, 2019 11:15

add service config pre-parsing support

3dbc828

iterate

f1f80b2

dfawley force-pushed the lbconfig branch from 11472c4 to 8535d98 Compare May 24, 2019 18:15

dfawley added 2 commits May 24, 2019 13:18

grpclb

8de4b4e

lbconfig

9f410da

dfawley force-pushed the lbconfig branch from 8535d98 to 9f410da Compare May 24, 2019 22:31

dfawley commented May 24, 2019

View reviewed changes

dfawley assigned menghanl and unassigned dfawley May 28, 2019

menghanl approved these changes May 29, 2019

View reviewed changes

should never happen

2ff8918

dfawley commented May 29, 2019

View reviewed changes

dfawley merged commit d40a995 into grpc:master May 30, 2019

dfawley deleted the lbconfig branch May 30, 2019 16:13

dfawley added this to the 1.22 Release milestone May 30, 2019

dfawley mentioned this pull request Jun 5, 2019

Implement proposal A24: load balancing config support #2728

Closed

lock bot locked as resolved and limited conversation to collaborators Nov 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

balancer/resolver: add loadBalancingConfig and pre-parsing support #2732

balancer/resolver: add loadBalancingConfig and pre-parsing support #2732

dfawley commented Mar 29, 2019 •

edited

Loading

menghanl Apr 3, 2019

dfawley May 21, 2019

menghanl Apr 3, 2019

dfawley May 21, 2019

menghanl Apr 3, 2019

dfawley May 24, 2019

menghanl Apr 3, 2019

dfawley May 21, 2019

menghanl Apr 3, 2019

dfawley May 21, 2019

menghanl Apr 3, 2019

dfawley May 21, 2019

dfawley left a comment

dfawley May 21, 2019

dfawley May 21, 2019

dfawley May 21, 2019

dfawley May 21, 2019

dfawley May 21, 2019

dfawley May 24, 2019

dfawley May 24, 2019 •

edited

Loading

menghanl left a comment

dfawley left a comment

balancer/resolver: add loadBalancingConfig and pre-parsing support #2732

balancer/resolver: add loadBalancingConfig and pre-parsing support #2732

Conversation

dfawley commented Mar 29, 2019 • edited Loading

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

dfawley left a 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

dfawley May 24, 2019 • edited Loading

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

dfawley commented Mar 29, 2019 •

edited

Loading

dfawley May 24, 2019 •

edited

Loading