Register and use default balancers and resolvers #1551

menghanl · 2017-10-02T22:16:45Z

The default balancer will be pickfirst if no balancer is specified.
And roundrobin is registered by default.

When switching balancer, the old balancer will be closed first, and a new balancer will start.
There's a gap between, where we will do optimization later.

Also removed two tests:

TestDialWithBlockErrorOnBadCertificates
TestDialWithBlockErrorOnNonTemporaryErrorDialer

$switch-balancer-1$

dfawley · 2017-10-16T21:02:09Z

balancer/roundrobin/roundrobin_test.go

@@ -99,7 +100,11 @@ func TestOneBackend(t *testing.T) {
 	}
 	defer test.cleanup()

-	cc, err := grpc.Dial(r.Scheme()+":///test.server", grpc.WithInsecure(), grpc.WithBalancerBuilder(newBuilder()))
+	rr := balancer.Get("roundrobin")


package balancer const RoundRobin = "roundrobin" // or var RoundRobinBuilder = ....

?

If not, I think a const in this test would be better than repeatedly using the same string.

Added a const (a var actually) to roundrobin_test.

In theory, a user can re-register a roundrobin and Get will return the new one. So a exported variable in balancer package won't work.

dfawley · 2017-10-16T21:05:32Z

balancer_switching_test.go

+		reply string
+		err   error
+	)
+	// The second RPC should succeed with the first server.


I don't understand this comment... Why doesn't the first RPC succeed with the first server? Why try 1000 times if the second one should succeed?

Should we do several attempts that all return the same server? I.e. do calls until you get the one you want, then do ~3 more and make sure they all match the same one? (So we're sure we aren't round-robining.)

dfawley · 2017-10-16T21:10:25Z

balancer_switching_test.go

+		for _, s := range servers {
+			var up bool
+			for i := 0; i < 1000; i++ {
+				if err = Invoke(context.Background(), "/foo/bar", &req, &reply, cc); err != nil && ErrorDesc(err) == s.port {


So, this does 1k RPCs until it eventually sees the server it's looking for, then it repeats through each server.. Why do 1k attempts? Maybe the second run-through should only do a single attempt for each, since we know we have a connection to every server by then?

This is not the real check. This is to make sure each server will be picked (all the subConns are connected).
The following loop does the real roundrobin check.

dfawley · 2017-10-16T21:16:10Z

clientconn.go

+		cc.mu.Unlock()
+		return
+	}
+	defer cc.mu.Unlock()


If you move this above 626, then you don't need to do it inside the if

dfawley · 2017-10-16T21:18:28Z

clientconn.go

+	grpclog.Infof("ClientConn switching balancer to %q", name)
+
+	if cc.customBalancer {
+		grpclog.Infoln("failed to switch balancer because custom balancer was set")


Suggestion: I'd like to avoid "failed" here. Maybe something like "ignoring service config balancer configuration: WithBalancer DialOption used instead"

dfawley · 2017-10-16T21:20:32Z

clientconn.go

+		cc.mu.Unlock()
+		return
+	}
+	defer cc.mu.Unlock()


Move this defer to the end and make it not a defer? Or move it to the top and remove the direct call on 659?

FWIW, defers are fine (and help with readability) as long as they're not on critical paths / high-frequency operations.

menghanl

Thanks for the review. PTAL.

menghanl · 2017-10-16T21:36:34Z

balancer/roundrobin/roundrobin_test.go

@@ -99,7 +100,11 @@ func TestOneBackend(t *testing.T) {
 	}
 	defer test.cleanup()

-	cc, err := grpc.Dial(r.Scheme()+":///test.server", grpc.WithInsecure(), grpc.WithBalancerBuilder(newBuilder()))
+	rr := balancer.Get("roundrobin")


Added a const (a var actually) to roundrobin_test.

In theory, a user can re-register a roundrobin and Get will return the new one. So a exported variable in balancer package won't work.

menghanl · 2017-10-16T21:37:55Z

clientconn.go

+		cc.mu.Unlock()
+		return
+	}
+	defer cc.mu.Unlock()


menghanl · 2017-10-16T21:38:32Z

clientconn.go

+	grpclog.Infof("ClientConn switching balancer to %q", name)
+
+	if cc.customBalancer {
+		grpclog.Infoln("failed to switch balancer because custom balancer was set")


menghanl · 2017-10-16T21:39:11Z

clientconn.go

+		cc.mu.Unlock()
+		return
+	}
+	defer cc.mu.Unlock()


menghanl · 2017-10-16T21:53:12Z

balancer_switching_test.go

+		reply string
+		err   error
+	)
+	// The second RPC should succeed with the first server.


menghanl · 2017-10-16T21:55:08Z

balancer_switching_test.go

+		for _, s := range servers {
+			var up bool
+			for i := 0; i < 1000; i++ {
+				if err = Invoke(context.Background(), "/foo/bar", &req, &reply, cc); err != nil && ErrorDesc(err) == s.port {


This is not the real check. This is to make sure each server will be picked (all the subConns are connected).
The following loop does the real roundrobin check.

removed two tests: - TestDialWithBlockErrorOnBadCertificates - TestDialWithBlockErrorOnNonTemporaryErrorDialer

dfawley · 2017-10-17T17:10:27Z

clientconn.go

+		cc.mu.Unlock()
+		return
+	}
+	defer cc.mu.Unlock()


FWIW, defers are fine (and help with readability) as long as they're not on critical paths / high-frequency operations.

dfawley · 2017-10-17T17:12:13Z

balancer_switching_test.go

+		for _, s := range servers {
+			var up bool
+			for i := 0; i < 1000; i++ {
+				if err = Invoke(context.Background(), "/foo/bar", &req, &reply, cc); err != nil && ErrorDesc(err) == s.port {


thelinuxfoundation · 2017-10-17T22:36:45Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @menghanl isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
The Linux Foundation CLA GitHub bot

menghanl force-pushed the bar_default_b_a_r branch from 7ab4a99 to d28c807 Compare October 2, 2017 22:49

menghanl changed the title ~~Bar default b a r~~ Register and use default balancers and resolvers Oct 2, 2017

menghanl force-pushed the bar_default_b_a_r branch 6 times, most recently from 15c0d49 to bd21314 Compare October 3, 2017 18:44

menghanl requested a review from dfawley October 3, 2017 20:07

menghanl mentioned this pull request Oct 3, 2017

Make interop client dial blocking #1559

Merged

dfawley self-assigned this Oct 4, 2017

dfawley reviewed Oct 16, 2017

View reviewed changes

menghanl commented Oct 16, 2017

View reviewed changes

menghanl mentioned this pull request Oct 16, 2017

Is balancer api v2 stable? #1579

Closed

menghanl added 6 commits October 16, 2017 16:33

make default pickfirst and switching balancer

9e0365b

removed two tests: - TestDialWithBlockErrorOnBadCertificates - TestDialWithBlockErrorOnNonTemporaryErrorDialer

comments and tests fixing

40cc8e6

fix import cycle and tests

801e01f

fix flaky test switch balancer and race

c34bfba

fix comments

a3f4d22

fix comments

c9c016c

menghanl force-pushed the bar_default_b_a_r branch from eefdb4d to c9c016c Compare October 16, 2017 23:33

fix build failure after rebase

641c02c

dfawley approved these changes Oct 17, 2017

View reviewed changes

fix defer

2331ba0

menghanl added 3 commits October 17, 2017 16:36

fix flaky check pickfirst

c88ac5d

Merge branch 'master' into bar_default_b_a_r

28a6da1

Merge branch 'master' into bar_default_b_a_r

652ac54

menghanl merged commit a353537 into grpc:master Oct 19, 2017

menghanl deleted the bar_default_b_a_r branch October 19, 2017 18:32

menghanl added a commit to menghanl/grpc-go that referenced this pull request Oct 25, 2017

Register and use default balancers and resolvers (grpc#1551)

0c4b6f9

menghanl added this to the 1.8 Release milestone Nov 7, 2017

menghanl added the Type: Feature New features or improvements in behavior label Nov 7, 2017

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

Register and use default balancers and resolvers #1551

Register and use default balancers and resolvers #1551

menghanl commented Oct 2, 2017 •

edited

Loading

dfawley Oct 16, 2017

menghanl Oct 16, 2017

dfawley Oct 16, 2017

menghanl Oct 16, 2017

dfawley Oct 16, 2017

menghanl Oct 16, 2017

dfawley Oct 17, 2017

dfawley Oct 16, 2017

menghanl Oct 16, 2017

dfawley Oct 16, 2017

menghanl Oct 16, 2017

dfawley Oct 16, 2017

menghanl Oct 16, 2017

dfawley Oct 17, 2017

menghanl left a comment

menghanl Oct 16, 2017

menghanl Oct 16, 2017

menghanl Oct 16, 2017

menghanl Oct 16, 2017

menghanl Oct 16, 2017

menghanl Oct 16, 2017

dfawley Oct 17, 2017

dfawley Oct 17, 2017

thelinuxfoundation commented Oct 17, 2017

Register and use default balancers and resolvers #1551

Register and use default balancers and resolvers #1551

Conversation

menghanl commented Oct 2, 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

thelinuxfoundation commented Oct 17, 2017

menghanl commented Oct 2, 2017 •

edited

Loading