addrConn: change address to slice of address #1376

ZhouyihaiDing · 2017-07-18T21:22:16Z

Change addrConn.addr:Address to addrConn.addrs:[]Address. cc.resetAddrConn receives []Address to create addrConn. All address shares the same addrConn/transport. ac.resetTransport will iterate from the beginning to find a usable address to create transport. Backoff time now is for all address fail, not just single address fail.
If current transport's address is in the slice, just update the slice without close and restart the transport to avoid reconnect. Otherwise reset the transport.
Wrap roundrobin balacer to pickfirst balancer, and use it for some test cases to test 1 and 2. addrConn will always use the first usable address in ac.addrs when balancer delete/add address or server down.

dfawley

Please rebase this and if it doesn't go pretty smoothly, let's refactor addrconn into its own file before working on this.

dfawley · 2017-07-20T20:22:50Z

clientconn.go

@@ -517,7 +517,7 @@ func (cc *ClientConn) lbWatcher(doneChan chan struct{}) {
 			}
 			if !keep {
 				del = append(del, c)
-				delete(cc.conns, c.addr)
+				delete(cc.conns, c.curAddr)


Should this not be addrs[0]?

Change it to k seems better.

dfawley · 2017-07-20T20:25:04Z

clientconn.go

-		ac.printf("ready")
-		if ac.state == Shutdown {
-			// ac.tearDown(...) has been invoked.
+			ac.curAddr = addr


Do we need to clear this ever?

Initiate it each time as "0" at the beginning of resetTransport. It also avoids the race when other thread want to use conn[current address] to find current addrconn.
And also set to back to "0" when calling ac.teardown()

dfawley · 2017-07-26T16:51:06Z

balancer.go

+
+// PickFirst Balancer is a simple balancer for testing multi-addresses in one addrConn.
+// By using this balancer, all address shares the same addrConn.
+// Although it wrapped by RoundRobin balancer, the logic of all methods work fine because


This should go on pickFirst instead, otherwise it will be visible in our godoc, but it's intended for readers of the code (not the API).

dfawley · 2017-07-26T16:52:10Z

balancer.go

+
+// The only difference is using ff.watchAddrUpdates() to use findFirstMD
+func (ff *pickFirst) Start(target string, config BalancerConfig) error {
+	return ff.rr.Start(target, config)


If you embed the rr balancer in pickFirst, you won't need to rewrite functions like this:

type pickFirst struct { *roundRobin }

dfawley · 2017-07-26T16:52:47Z

balancer.go

+	return &pickFirst{rr: &roundRobin{r: r}}
+}
+
+// The only difference is using ff.watchAddrUpdates() to use findFirstMD


Function comments must start with the name of the function.

In this case, you should just leave them out because the type (and, consequently this function) are not exported.

dfawley · 2017-07-26T16:54:47Z

balancer.go

+}
+
+// The only difference is using ff.watchAddrUpdates() to use findFirstMD
+func (ff *pickFirst) Start(target string, config BalancerConfig) error {


pickFirst -> pf for receiver name?

Oh, I'll change that. In my first implementation, I thought the word was findFirst, so I used ff.
And also I forgot to update the comment because I was using metadata type(findFirstMD) to decide which balancer in that version.

dfawley · 2017-07-26T16:56:18Z

balancer.go

+	return &pickFirst{rr: &roundRobin{r: r}}
+}
+
+// The only difference is using ff.watchAddrUpdates() to use findFirstMD


Also, I think this comment is stale. watchAddrUpdates() wasn't changed and Start just calls the RR balancer's Start.

I think you can comment pickFirst as:

pickFirst is the same as roundRobin, but with a different type to control conditional behavior in grpc internals.

dfawley · 2017-07-26T17:00:48Z

clientconn.go

-		for _, a := range addrs {
-			if _, ok := cc.conns[a]; !ok {
-				add = append(add, a)
+		for _, addr := range addrs {


This change is gratuitous and go style prefers shorter variable names for small loops like this (as it was before); please revert.

dfawley · 2017-07-26T17:04:57Z

clientconn.go

+	// not work for roundrobin
+	cc.mu.Lock()
+	if len(cc.conns) != 0 && (reflect.TypeOf(cc.dopts.balancer) == reflect.TypeOf(&pickFirst{})) {
+		var currentAc *addrConn


This and the next 4 lines can be currentAc := cc.conns[0], no?

cc.conns is a map, not slice. This would work:

var currentAc *addrConn for _, currentAc = range cc.conns { break }

dfawley · 2017-07-26T17:09:23Z

clientconn.go

+			currentAc = v
+			break
+		}
+		var addrInUse bool


Consider moving the rest of this logic to a method in addrConn (UpdateAddresses).

dfawley · 2017-07-26T17:10:36Z

clientconn.go

-	stale := cc.conns[ac.addr]
-	cc.conns[ac.addr] = ac
+	stale := cc.conns[ac.addrs[0]]
+	for i := 0; i < len(ac.addrs); i++ {


for _, a := range ac.addrs {
...

dfawley · 2017-07-26T17:13:33Z

clientconn.go

+		// copy ac.addrs in case of race
+		addrsIter := make([]Address, len(ac.addrs))
+		copy(addrsIter, ac.addrs)
+		ac.mu.Unlock()


Well, I was going to suggest this:

// No ac.mu.Unlock() for _, addr := range addrsIter { // No ac.mu.Lock() if state == shutdown ... ac.mu.Unlock() // as before ...... ac.mu.Lock() }

But the logic is so complicated (continues and returns) that that is hard.

Can the body of this loop be factored out into one or a few functions? It's quite long.

Returns won't change cause it has to be unlock before return anyway. But add some lock and unlock around continue and 'for' loop seems make the code hard to read and be modified next time.
Seems the second for loop (iterate each address) can create a single function like resetClientTransport([]Address) ?

menghanl · 2017-07-26T17:59:19Z

balancer.go

+}
+
+func (ff *pickFirst) Notify() <-chan []Address {
+	return ff.rr.addrCh


return ff.rr.Notify()

menghanl · 2017-07-26T18:01:11Z

clientconn.go

-		if err := cc.resetAddrConn(Address{Addr: target}, cc.dopts.block, nil); err != nil {
+		var inputaddr []Address
+		inputaddr = append(inputaddr, Address{Addr: target})
+		if err := cc.resetAddrConn(inputaddr, cc.dopts.block, nil); err != nil {


cc.resetAddrConn([]Address{{Addr: target}}, cc.dopts.block, nil)

menghanl · 2017-07-26T18:04:34Z

clientconn.go

+				close(doneChan)
+				doneChan = nil
+			}
+		} else { // isroundRobin, remain the same but change a to []Address{a}


Not pickfirst.

menghanl · 2017-07-26T18:18:33Z

clientconn.go

+	// not work for roundrobin
+	cc.mu.Lock()
+	if len(cc.conns) != 0 && (reflect.TypeOf(cc.dopts.balancer) == reflect.TypeOf(&pickFirst{})) {
+		var currentAc *addrConn


cc.conns is a map, not slice. This would work:

var currentAc *addrConn for _, currentAc = range cc.conns { break }

menghanl · 2017-07-27T20:48:57Z

clientconn.go

@@ -675,23 +694,81 @@ func (cc *ClientConn) scWatcher() {
 	}
 }

+// UpdateAddresses checks whether current address in the updating list, Update the list if true.
+func (cc *ClientConn) UpdateAddresses(addrs []Address) bool {


Do not export this function.

menghanl · 2017-07-27T20:51:31Z

clientconn.go

+	// not work for roundrobin
+	cc.mu.Lock()
+	_, isPickFirst := cc.dopts.balancer.(*pickFirst)
+	if isPickFirst {


if _, isPickFirst := cc.dopts.balancer.(*pickFirst); isPickFirst {

menghanl · 2017-07-27T21:06:21Z

balancer.go

+	*roundRobin
+}
+
+// PickFirstBalancer is a simple balancer for testing multi-addresses in one addrConn.


s/PickFirstBalancer/pickFirstBalancer

And add period at the end.

menghanl · 2017-07-27T21:09:11Z

clientconn.go

-		for k, c := range cc.conns {
-			var keep bool
+		} else {
+			// Not pickFirst. All remains the same but changing addr to []Address{addr}


This comment would look wired without the context of this PR. Try to make it more general, just describe what's happening.

Something like "Not pickFirst, create a new addrConn for each address."

menghanl · 2017-07-31T18:34:19Z

balancer.go

+	*roundRobin
+}
+
+// pickFirstBalancer is a simple balancer for testing multi-addresses in one addrConn.


Move this comment to type pickFirst.

menghanl · 2017-07-31T18:35:12Z

balancer.go

+// pickFirstBalancer is a simple balancer for testing multi-addresses in one addrConn.
+// By using this balancer, all address shares the same addrConn.
+// Although it wrapped by RoundRobin balancer, the logic of all methods work fine because
+// balancer. Get() returns the address Up by resetTransport().


Extra space before Get.

menghanl · 2017-07-31T18:35:32Z

balancer.go

+
+// pickFirstBalancer is a simple balancer for testing multi-addresses in one addrConn.
+// By using this balancer, all address shares the same addrConn.
+// Although it wrapped by RoundRobin balancer, the logic of all methods work fine because


Although it's a wrapper around RoundRobin?

menghanl · 2017-07-31T18:35:48Z

clientconn.go

+					ac.ready = nil
+				}
+				ac.mu.Unlock()
+				select {


remove this select.

Don't we need to check ac.ctx.Done()? Cause eg. there are 6 address and the ac.ctx.Done() return at the 2nd loop, the rest 4 loops don't need to continue?

menghanl · 2017-07-31T18:52:54Z

clientconn.go

+				return errConnClosing
+			}
+			ac.mu.Unlock()
+			timeout := minConnectTimeout


Move this before the for loop? We only need to calculate this once.

menghanl · 2017-07-31T18:57:18Z

clientconn.go

@@ -1150,6 +1245,7 @@ func (ac *addrConn) tearDown(err error) {
 	ac.cancel()

 	ac.mu.Lock()
+	ac.curAddr = Address{Addr: "0"}


Make curAddr a pointer and set it to nil?

Done. Didn't know go can use nil as key.

menghanl · 2017-08-11T18:36:40Z

clientconn.go

+			cc.conns[addr] = currentAc
+		}
+		currentAc.addrs = addrs
+		//cc.mu.Unlock()


Remove this.

menghanl · 2017-08-11T18:48:51Z

clientconn.go

@@ -675,23 +694,81 @@ func (cc *ClientConn) scWatcher() {
 	}
 }

+// addressesUpdated checks whether current address in the updating list, Update the list if true.
+func (cc *ClientConn) addressesUpdated(addrs []Address) bool {


Name this as pickFirstUpdateAddresses() and comment that it's only used when balancer is pickfirst.

dfawley

Just a couple nits about comments.

dfawley · 2017-08-15T17:21:28Z

balancer.go

@@ -395,3 +395,14 @@ func (rr *roundRobin) Close() error {
 	}
 	return nil
 }
+
+// pickFirst is used to test multi-addresses in one addrConn which all address shares the same addrConn.


... in which all addresses share ...

dfawley · 2017-08-15T17:21:46Z

balancer.go

@@ -395,3 +395,14 @@ func (rr *roundRobin) Close() error {
 	}
 	return nil
 }
+
+// pickFirst is used to test multi-addresses in one addrConn which all address shares the same addrConn.
+// It wrappers around roundRobin balancer. The logic of all methods works fine because balancer.Get()


"It wraps around" or "It is a wrapper around"

* addrConn: change address to slice of address * add pickfirst balancer to test new addrconn

dfawley reviewed Jul 20, 2017

View reviewed changes

ZhouyihaiDing force-pushed the addrcon branch 3 times, most recently from c062b44 to 5870195 Compare July 21, 2017 01:05

addrConn: change address to slice of address

9aca425

ZhouyihaiDing force-pushed the addrcon branch from 5870195 to 9aca425 Compare July 21, 2017 01:07

ZhouyihaiDing added 2 commits July 21, 2017 11:27

add pickfirst balancer to test new addrconn

6c3802c

merge conflicts

79d8768

ZhouyihaiDing force-pushed the addrcon branch from 4af33d1 to c9f3ab2 Compare July 22, 2017 00:09

fix race issues

cf20124

ZhouyihaiDing force-pushed the addrcon branch 20 times, most recently from 3004921 to 9ea895d Compare July 23, 2017 00:44

ZhouyihaiDing force-pushed the addrcon branch from 160cbcc to cb61eea Compare July 24, 2017 19:03

dfawley reviewed Jul 26, 2017

View reviewed changes

menghanl reviewed Jul 26, 2017

View reviewed changes

update according to comments

8dd35b4

ZhouyihaiDing force-pushed the addrcon branch from 3ce7ccb to 75c875e Compare July 27, 2017 17:05

solving conflict

0410e70

ZhouyihaiDing force-pushed the addrcon branch from 75c875e to 0410e70 Compare July 27, 2017 17:15

format

181dc4f

ZhouyihaiDing force-pushed the addrcon branch from a8cee54 to 181dc4f Compare July 27, 2017 19:02

menghanl reviewed Jul 27, 2017

View reviewed changes

menghanl self-assigned this Jul 28, 2017

menghanl reviewed Jul 28, 2017

View reviewed changes

update after review

061729f

ZhouyihaiDing force-pushed the addrcon branch from 571b627 to 061729f Compare July 28, 2017 23:10

menghanl reviewed Jul 31, 2017

View reviewed changes

update by latest review

e3363f0

menghanl reviewed Aug 11, 2017

View reviewed changes

ZhouyihaiDing added 2 commits August 13, 2017 14:47

update

44c8b7c

solve conflict

65cf9da

menghanl approved these changes Aug 14, 2017

View reviewed changes

dfawley reviewed Aug 15, 2017

View reviewed changes

dfawley assigned ZhouyihaiDing Aug 17, 2017

update comments

0bff2f0

menghanl merged commit 2308131 into grpc:master Aug 21, 2017

menghanl added 1.6 Type: Bug labels Aug 24, 2017

dfawley modified the milestone: 1.6 Release Aug 28, 2017

menghanl pushed a commit to menghanl/grpc-go that referenced this pull request Aug 30, 2017

addrConn: change address to slice of address (grpc#1376)

839d7b3

* addrConn: change address to slice of address * add pickfirst balancer to test new addrconn

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

addrConn: change address to slice of address #1376

addrConn: change address to slice of address #1376

Conversation

ZhouyihaiDing commented Jul 18, 2017 • edited Loading

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

ZhouyihaiDing Jul 27, 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

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

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

ZhouyihaiDing commented Jul 18, 2017 •

edited

Loading

ZhouyihaiDing Jul 27, 2017 •

edited

Loading