Switch balancer to grpclb when at least one address is grpclb address #1692

menghanl · 2017-11-28T00:09:33Z

$switch-balancer-1$

This change is

dfawley · 2017-12-05T18:05:33Z

clientconn.go

@@ -600,6 +600,7 @@ type ClientConn struct {
 	// Keepalive parameter can be updated if a GoAway is received.
 	mkp             keepalive.ClientParameters
 	curBalancerName string
+	preBalancerName string // previous balancer nam.


dfawley · 2017-12-05T18:45:37Z

clientconn.go

+			newBalancerName = pickfirstName
+		}
+		for _, a := range addrs {
+			if a.Type == resolver.GRPCLB && newBalancerName != grpclbName {


I think we don't want "newBalancerName != grpclbName". We should break if any Type is resolver.GRPCLB, and re-assigning newBalancerName to grpclbName is not worth optimizing away (actually it's more optimal than the extra branch).

dfawley · 2017-12-05T18:46:40Z

clientconn.go

+				// Switch from non-grpclb to grpclb.
+				newBalancerName = grpclbName
+				break
+			} else if a.Type == resolver.Backend && newBalancerName == grpclbName {


I thought we wanted to go with gRPCLB if any address is a gRPCLB address? This will switch away if both kinds are returned and we were previously using gRPCLB.

Maybe we should have a test where the gRPCLB backend changes (but both before and after are gRPCLB).

Right!
Fixed and added a test.

dfawley · 2017-12-05T18:47:28Z

clientconn.go

 	cc.balancerWrapper.handleResolvedAddrs(addrs, nil)
 }

-// switchBalancer starts the switching from current balancer to the balancer with name.
+// switchBalancer starts the switching from current balancer to the balancer
+// with the given name. It will NOT send the current address list to the new


It will NOT send the current address list to the new balancer.

Why? How do we bootstrap the new balancer then?

Caller of this function should do that. Comment updated.

dfawley · 2017-12-05T18:49:48Z

clientconn.go

@@ -688,7 +700,6 @@ func (cc *ClientConn) switchBalancer(name string) {
 		grpclog.Infoln("ignoring service config balancer configuration: WithBalancer DialOption used instead")
 		return
 	}
-
 	if cc.curBalancerName == name {


Should this go above the log about switching?

The idea here is, if balancer is set by a dial option, all switch balancer call will be no-op.

Right, but we don't need to log that we aren't switching if the name doesn't change, right?

dfawley · 2017-12-05T18:54:27Z

balancer_switching_test.go

+		time.Sleep(time.Millisecond)
+	}
+	if !isPickFirst {
+		t.Fatalf("after 1 second, cc.balancer is of type %v, not pick_first", cc.curBalancerName)


Is 1s long enough for these tests on travis? I'd rather give them 5-10 to avoid potential flakiness -- they stop early when the condition occurs, so it's not harmful to give them longer unless the timing is important.

menghanl

Thanks for the review.
All done. PTAL.

menghanl · 2017-12-07T01:41:12Z

clientconn.go

@@ -600,6 +600,7 @@ type ClientConn struct {
 	// Keepalive parameter can be updated if a GoAway is received.
 	mkp             keepalive.ClientParameters
 	curBalancerName string
+	preBalancerName string // previous balancer nam.


menghanl · 2017-12-07T01:42:55Z

clientconn.go

 	cc.balancerWrapper.handleResolvedAddrs(addrs, nil)
 }

-// switchBalancer starts the switching from current balancer to the balancer with name.
+// switchBalancer starts the switching from current balancer to the balancer
+// with the given name. It will NOT send the current address list to the new


Caller of this function should do that. Comment updated.

menghanl · 2017-12-07T01:54:26Z

clientconn.go

@@ -688,7 +700,6 @@ func (cc *ClientConn) switchBalancer(name string) {
 		grpclog.Infoln("ignoring service config balancer configuration: WithBalancer DialOption used instead")
 		return
 	}
-
 	if cc.curBalancerName == name {


The idea here is, if balancer is set by a dial option, all switch balancer call will be no-op.

menghanl · 2017-12-08T19:19:54Z

balancer_switching_test.go

+		time.Sleep(time.Millisecond)
+	}
+	if !isPickFirst {
+		t.Fatalf("after 1 second, cc.balancer is of type %v, not pick_first", cc.curBalancerName)


menghanl · 2017-12-08T19:39:37Z

clientconn.go

+				// Switch from non-grpclb to grpclb.
+				newBalancerName = grpclbName
+				break
+			} else if a.Type == resolver.Backend && newBalancerName == grpclbName {


Right!
Fixed and added a test.

menghanl · 2017-12-08T22:55:13Z

clientconn.go

+			newBalancerName = pickfirstName
+		}
+		for _, a := range addrs {
+			if a.Type == resolver.GRPCLB && newBalancerName != grpclbName {


dfawley · 2017-12-09T00:35:19Z

clientconn.go

+		}
+		var newBalancerName string
+		if isGRPCLB {
+			newBalancerName = grpclbName


Optional, but it's a go-ism:

newBalancerName := grpclbName if !isGRPCLB { ... }

but it's a little awkward in this case, so either is probably fine.

dfawley · 2017-12-11T17:01:42Z

clientconn.go

@@ -719,15 +719,14 @@ func (cc *ClientConn) switchBalancer(name string) {
 	}
 	grpclog.Infof("ClientConn switching balancer to %q", name)

-	if cc.dopts.balancerBuilder != nil {
-		grpclog.Infoln("ignoring balancer switching: WithBalancer DialOption used instead")
+	if strings.ToLower(cc.curBalancerName) == strings.ToLower(name) {


I would rather move this above the info log on 720 also, but if you feel strongly then OK.

menghanl requested a review from dfawley November 28, 2017 00:09

menghanl mentioned this pull request Nov 29, 2017

New balancer and resolver APIs #1388

Closed

8 tasks

dfawley self-assigned this Nov 30, 2017

menghanl assigned menghanl and unassigned dfawley Dec 1, 2017

menghanl force-pushed the switch_to_grpclb branch from dbd74c5 to d58d623 Compare December 1, 2017 23:27

menghanl assigned dfawley and unassigned menghanl Dec 1, 2017

dfawley reviewed Dec 5, 2017

View reviewed changes

menghanl added 3 commits December 6, 2017 17:40

Switch balancer to grpclb when at least one address is grpclb address

5cf4f10

build dial config balancer in handleResolvedAddrs

ec08303

service config should not switch balancer if current is grpclb

2e1f9c7

dfawley assigned menghanl and unassigned dfawley Dec 7, 2017

fix comments

4c706ca

menghanl force-pushed the switch_to_grpclb branch from d7b0bf5 to 4c706ca Compare December 8, 2017 23:23

menghanl commented Dec 8, 2017

View reviewed changes

menghanl assigned dfawley and unassigned menghanl Dec 8, 2017

dfawley approved these changes Dec 9, 2017

View reviewed changes

move code to avoid unnecessary log

bb9abf2

dfawley reviewed Dec 11, 2017

View reviewed changes

move code to avoid unnecessary log, 2

b91f056

menghanl merged commit dba60db into grpc:master Dec 12, 2017

menghanl deleted the switch_to_grpclb branch December 12, 2017 20:45

dfawley added the Type: Feature New features or improvements in behavior label Jan 2, 2018

dfawley added this to the 1.9 Release milestone Jan 2, 2018

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch balancer to grpclb when at least one address is grpclb address #1692

Switch balancer to grpclb when at least one address is grpclb address #1692

menghanl commented Nov 28, 2017 •

edited

Loading

dfawley Dec 5, 2017

menghanl Dec 7, 2017

dfawley Dec 5, 2017

menghanl Dec 8, 2017

dfawley Dec 5, 2017

menghanl Dec 8, 2017

dfawley Dec 5, 2017

menghanl Dec 7, 2017

dfawley Dec 5, 2017

menghanl Dec 7, 2017

dfawley Dec 9, 2017

menghanl Dec 9, 2017

dfawley Dec 5, 2017

menghanl Dec 8, 2017

menghanl left a comment

menghanl Dec 7, 2017

menghanl Dec 7, 2017

menghanl Dec 7, 2017

menghanl Dec 8, 2017

menghanl Dec 8, 2017

menghanl Dec 8, 2017

dfawley Dec 9, 2017

dfawley Dec 11, 2017

menghanl Dec 11, 2017

Switch balancer to grpclb when at least one address is grpclb address #1692

Switch balancer to grpclb when at least one address is grpclb address #1692

Conversation

menghanl commented Nov 28, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl commented Nov 28, 2017 •

edited

Loading