New grpclb implementation #1558

menghanl · 2017-10-03T20:58:38Z

Also add new feature:

Fallback to backends if remote balancer is unavailable

$new-bar-2$

dfawley · 2017-10-20T21:27:00Z

balancer/balancer.go

@@ -128,6 +128,10 @@ type PickOptions struct{}
 type DoneInfo struct {
 	// Err is the rpc error the RPC finished with. It could be nil.
 	Err error
+	// BytesSent indicates if any byte has been sent to the server.


*"bytes have"

dfawley · 2017-10-20T21:34:26Z

call.go

@@ -251,13 +251,14 @@ func invoke(ctx context.Context, method string, args, reply interface{}, cc *Cli
 		stream, err = t.NewStream(ctx, callHdr)
 		if err != nil {
 			if done != nil {
+				doneInfo := balancer.DoneInfo{Err: err}
 				if _, ok := err.(transport.ConnectionError); ok {
 					// If error is connection error, transport was sending data on wire,


This is actually not true (any longer?).

NewStream only returns errors before it attempts to write to the network. I.e. this block should be deleted (and is by #1597).

Ping? Bytes not sent.

Rebased on top of the retry PR. This was removed.

dfawley · 2017-10-20T21:35:47Z

call.go

-				done(balancer.DoneInfo{Err: err})
+				doneInfo := balancer.DoneInfo{
+					Err:           err,
+					BytesSent:     stream.BytesSent(),


Similarly, if there is no error from NewStream, we always attempted to write bytes to the wire. So this should be "true". (And below.)

dfawley · 2017-10-20T21:37:27Z

grpclb_picker.go

+	NumCallsFinishedKnownReceived            int64
+}
+
+// toClientStats converts rpcStats to lbpb.ClientStats, and clear rpcStats.


dfawley · 2017-10-20T21:39:40Z

grpclb_picker.go

+
+	mu         sync.Mutex
+	serverList []*lbpb.Server
+	nextSL     int


What is nextSL and nextSC? Please document.

dfawley · 2017-10-20T22:45:44Z

grpclb.go

-	if l == nil {
+// regeneratePicker takes a snapshot of the balancer, and generate a picker from
+// it. The picker
+//  - always return ErrTransientFailure if the balancer is in TransientFailure,


*returnS
and doES below.

dfawley · 2017-10-20T22:48:23Z

grpclb.go

+func (lb *lbBalancer) fallbackToBackendsAfter(fallbackTimeout time.Duration) {
+	timer := time.NewTimer(fallbackTimeout)
+	defer func() {
+		if !timer.Stop() {


IIUC, timer is a local, so you don't even have to stop it if you don't want. You definitely don't have to check its return value and drain its channel. That's only if you want to reuse it.

dfawley · 2017-10-20T22:50:31Z

grpclb.go

-				// Has iterated all the possible address but none is connected.
-				break
-			}
+	var (


var remoteBalancerAddrs, backendAddrs []resolver.Address

?

dfawley · 2017-10-20T22:51:57Z

grpclb.go

-			b.mu.Unlock()
+	if lb.ccRemoteLB == nil {
+		if len(remoteBalancerAddrs) <= 0 {
+			grpclog.Fatalf("grpclb: no remote balancer address is available, should never happen")


Can we return an error instead anyway?

done

We don't check this returned error, though...

dfawley · 2017-10-20T22:53:01Z

grpclb.go

 	}
-	if b.w != nil {
-		b.w.Close()
+	close(lb.doneCh)


This is still racy - is that OK?

Close() is guaranteed to be called from one goroutine (never in parallel).
So this should be OK.

menghanl

Thanks for the review. PTAL.

menghanl · 2017-10-23T18:48:13Z

balancer/balancer.go

@@ -128,6 +128,10 @@ type PickOptions struct{}
 type DoneInfo struct {
 	// Err is the rpc error the RPC finished with. It could be nil.
 	Err error
+	// BytesSent indicates if any byte has been sent to the server.


menghanl · 2017-10-23T18:53:33Z

grpclb.go

-	}
+// NewLBBuilder creates a builder for grpclb.
+func NewLBBuilder() balancer.Builder {
+	// TODO(bar grpclb) this function is exported for testing only, remove it when resolver supports selecting grpclb.


menghanl · 2017-10-23T20:47:49Z

grpclb.go

+// NewLBBuilderWithFallbackTimeout creates a grpclb builder with the given
+// fallbackTimeout.
+//
+// Only call this function when a non-default fallback timeout is needed.


This can also be used to override the registered grpclb builder with a new fallback timeout.
Updated the comment.

menghanl · 2017-10-23T20:48:39Z

grpclb.go

 }

-func (b *grpclbBalancer) processServerList(l *lbmpb.ServerList, seq int) {
-	if l == nil {
+// regeneratePicker takes a snapshot of the balancer, and generate a picker from


menghanl · 2017-10-23T20:49:08Z

grpclb.go

-	if l == nil {
+// regeneratePicker takes a snapshot of the balancer, and generate a picker from
+// it. The picker
+//  - always return ErrTransientFailure if the balancer is in TransientFailure,


menghanl · 2017-10-26T00:07:02Z

grpclb_remote_balancer.go

+			continue
+		}
+		if initResp.LoadBalancerDelegate != "" {
+			grpclog.Fatalf("TODO: Delegation is not supported yet.")


Changed to Errorf and continue.

menghanl · 2017-10-26T00:11:12Z

grpclb_remote_balancer.go

+}
+
+func (lb *lbBalancer) dialRemoteLB(remoteLBName string) {
+	var (


menghanl · 2017-10-26T00:11:44Z

grpclb_remote_balancer.go

+			grpclog.Fatalf("TODO: Delegation is not supported yet.")
+		}
+
+		// streamDone will be closed by the readServerList goroutine when


Hmm, right, context.
There's no need to create a new context here.
SendLoadReport() should block on the stream's context.

menghanl · 2017-10-26T00:18:25Z

grpclb_remote_balancer.go

+		// time.Ticker forever.
+		streamDone := make(chan struct{})
+
+		var wg sync.WaitGroup


menghanl · 2017-10-26T00:22:46Z

grpclb_remote_balancer.go

+		dopts = append(dopts, WithInsecure())
+	}
+	if lb.opt.Dialer != nil {
+		// WithDialer takes a different type of function, so we instead use a special DialOption here.


done.
Added a unexported withContextDialer()

dfawley · 2017-11-03T17:47:44Z

call.go

@@ -251,13 +251,14 @@ func invoke(ctx context.Context, method string, args, reply interface{}, cc *Cli
 		stream, err = t.NewStream(ctx, callHdr)
 		if err != nil {
 			if done != nil {
+				doneInfo := balancer.DoneInfo{Err: err}
 				if _, ok := err.(transport.ConnectionError); ok {
 					// If error is connection error, transport was sending data on wire,


Ping? Bytes not sent.

dfawley · 2017-11-21T17:10:12Z

grpclb.go

-	}
+// NewLBBuilder creates a builder for grpclb.
+func NewLBBuilder() balancer.Builder {
+	// TODO(bar grpclb) this function is exported for testing only, remove it when resolver supports selecting grpclb.


dfawley · 2017-11-21T17:11:07Z

grpclb.go

+// NewLBBuilderWithFallbackTimeout creates a grpclb builder with the given
+// fallbackTimeout.
+//
+// Only call this function when a non-default fallback timeout is needed.


What's a fallback timeout? OK, I think I know what it means, but can you explain in the comment?

dfawley · 2017-11-21T17:12:12Z

grpclb.go

+	// The ClientConn to talk to the remote balancer.
+	ccRemoteLB *ClientConn
+
+	mu sync.Mutex


What does this guard?

Everything following until the blank line...

I move the code around and added a comment.

dfawley · 2017-11-21T17:12:37Z

grpclb.go

 }

-func (b *grpclbBalancer) processServerList(l *lbmpb.ServerList, seq int) {
-	if l == nil {
+// regeneratePicker takes a snapshot of the balancer, and generate a picker from


Not done? Maybe your latest updates aren't pushed?

menghanl

Done. PTAL.

The "Bytes not sent" was about the byteSent boolean variable you removed in your retry change.
I rebased this PR on the retry commit, so it's fixed now.

menghanl · 2017-11-21T21:17:13Z

grpclb.go

 }

-func (b *grpclbBalancer) processServerList(l *lbmpb.ServerList, seq int) {
-	if l == nil {
+// regeneratePicker takes a snapshot of the balancer, and generate a picker from


Done again... And also the following one.

menghanl · 2017-11-21T21:19:06Z

grpclb.go

+// NewLBBuilderWithFallbackTimeout creates a grpclb builder with the given
+// fallbackTimeout.
+//
+// Only call this function when a non-default fallback timeout is needed.


menghanl · 2017-11-21T21:19:15Z

grpclb.go

-	}
+// NewLBBuilder creates a builder for grpclb.
+func NewLBBuilder() balancer.Builder {
+	// TODO(bar grpclb) this function is exported for testing only, remove it when resolver supports selecting grpclb.


Done again...

menghanl · 2017-11-21T21:20:54Z

grpclb.go

+	// The ClientConn to talk to the remote balancer.
+	ccRemoteLB *ClientConn
+
+	mu sync.Mutex


Everything following until the blank line...

I move the code around and added a comment.

split lbPicker into lbPicker and errPicker grpclb_remote_balancer.go go style and supress error logs add rrpicker

dfawley · 2017-11-22T00:04:50Z

grpclb_remote_balancer.go

 	for {
 		select {
 		case <-lb.doneCh:
 			return
 		default:
+			if remoteBalancerErr != nil {
+				grpclog.Error(remoteBalancerErr)
+			}


remoteBalancerErr = nil?

menghanl requested a review from dfawley October 3, 2017 23:35

dfawley self-assigned this Oct 4, 2017

dfawley mentioned this pull request Oct 5, 2017

flaky TestGRPCLBStatsStreamingDropLoadBalancing #1557

Closed

menghanl mentioned this pull request Oct 16, 2017

Is balancer api v2 stable? #1579

Closed

menghanl force-pushed the bar_new_grpclb branch from 94b3cd9 to e5af9ad Compare October 20, 2017 21:25

dfawley reviewed Oct 20, 2017

View reviewed changes

dfawley assigned menghanl and unassigned dfawley Oct 20, 2017

menghanl force-pushed the bar_new_grpclb branch from 3347ebc to 23c3be9 Compare October 26, 2017 00:27

menghanl commented Oct 26, 2017

View reviewed changes

dfawley assigned dfawley and unassigned menghanl Oct 26, 2017

menghanl force-pushed the bar_new_grpclb branch from 23c3be9 to 3b7168a Compare October 30, 2017 21:44

menghanl added 4 commits November 6, 2017 13:58

New implementation for grpclb

fe46d65

fix weighted roundrobin test

7354850

reviews

636c770

fix done error in unary rpcs

25da9c2

menghanl force-pushed the bar_new_grpclb branch from 3b7168a to 25da9c2 Compare November 6, 2017 21:58

fix compile error after rebase

aea4d55

dfawley reviewed Nov 21, 2017

View reviewed changes

menghanl commented Nov 21, 2017

View reviewed changes

menghanl added 2 commits November 21, 2017 13:24

done again...

0492762

review comments

9161131

split lbPicker into lbPicker and errPicker grpclb_remote_balancer.go go style and supress error logs add rrpicker

menghanl force-pushed the bar_new_grpclb branch from 479269c to 9161131 Compare November 21, 2017 23:37

dfawley reviewed Nov 22, 2017

View reviewed changes

menghanl added 2 commits November 21, 2017 16:16

refactor watchRemoteBalancer to supress logs

7bd2d00

add withResolverBuilder and use it in grpclb

15d0543

menghanl force-pushed the bar_new_grpclb branch from 9877aa1 to 15d0543 Compare November 22, 2017 17:59

dfawley approved these changes Nov 22, 2017

View reviewed changes

menghanl merged commit 2ef021f into grpc:master Nov 27, 2017

menghanl deleted the bar_new_grpclb branch November 27, 2017 21:35

menghanl mentioned this pull request Nov 29, 2017

New balancer and resolver APIs #1388

Closed

8 tasks

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

New grpclb implementation #1558

New grpclb implementation #1558

Conversation

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

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

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

menghanl commented Oct 3, 2017 •

edited

Loading