Pushsync retry next peer after error #947

zbiljic · 2020-11-16T18:51:39Z

Updates pushsync.PushChunkToClosest function with the change where it will attempt multiple peers if one fails.

Previous code only ever attempted to push to same peer (that was deemed as "closest"), even in the face of problems with that host. This change adds loop which attempts closest peer, and if there are some problems with that peer tries some other closest peer (from the available ones). Depending on the error, it may blocklist the peer for some time (one minute). It will try to maximum of 5 peers. If each of them fails it will return last error that was encountered.

Note that new code still uses original ClosestPeer function, in which case it will again be possible that selected peer will be "self". If we want to change this, it would be possible.

zbiljic · 2020-11-17T11:15:23Z

/run beekeeper

zbiljic · 2020-11-17T11:30:50Z

/run beekeeper

zbiljic · 2020-11-17T11:49:18Z

/run beekeeper

janos

Very nice. I have some comments only about defers in the retry loop.

janos · 2020-11-17T23:11:47Z

pkg/pushsync/pushsync.go

 		}
-		return nil, fmt.Errorf("closest peer: %w", err)
-	}
+		defer ps.accounting.Release(peer, receiptPrice)


This defer is inside a retry loop. I think that this call should happen after the current iteration ends, not after all iterations. Maybe to have some encapsulating function?

Both defer section I have left intentionally. I figured it would be fine to leave it for the end, however I see now it would be problem for accounting.

janos · 2020-11-17T23:13:38Z

pkg/pushsync/pushsync.go

+			ps.logger.Debugf("pushsync-push: %w", lastErr)
+			continue
+		}
+		defer func() { go streamer.FullClose() }()


If there are errors after this defer, there may be multiple FullClose calls. I do not think that it is a problem, but it is not necessary.

zelig

once again, i suggest handling technical debt asap after release

zelig · 2020-11-17T23:40:47Z

pkg/pushsync/pushsync.go

+			err  error
+		)
+
+		if i == 0 {


it is cleaner and easier to read if you pull this part before the for loop.

zelig · 2020-11-17T23:56:41Z

pkg/pushsync/pushsync.go

+		}
+		ps.metrics.ReceiptRTT.Observe(time.Since(receiptRTTTimer).Seconds())
+
+		// if you manage to get a tag, just increment the respective counter


this should come after sending off to the first peer. L307

I moved this intentionally after receiveReceipt section, as it seemed pointless to mark it as sent if we did not receive confirmation. If the host fails after that, we might switch to another, and increment sent state again. I will move it back.

actually you must not increment it twice #843 if you do you go above split count

I moved this intentionally after receiveReceipt section, as it seemed pointless to mark it as sent if we did not receive confirmation.

you made the wrong assumption. sent is incremented when the chunk hits the network
synced incremented when u get the receipt back (no matter how many retries it took)

zelig · 2020-11-18T00:03:27Z

pkg/pushsync/pushsync.go

+// provided address addr. This function will ignore peers with addresses
+// provided in skipPeers.
+func (ps *PushSync) closestPeer(addr swarm.Address, skipPeers []swarm.Address) (swarm.Address, error) {
+	closest := swarm.Address{}


name return arguments and automatically. get the zero value

I am aware of this, however I am not a great fan of named return arguments. I mostly use them to stay in line with some existing code.

hmm here you actually introduced one https://github.com/ethersphere/bee/pull/947/files#diff-1f5e1454bf78aa94ff061f30fb1b70e522169b317f53acdd13042a864561f41fR144

Well yes, as I commented, if there are usages in existing (surrounding) code, I try to fit new code to be similar.

zelig · 2020-11-18T00:08:55Z

pkg/pushsync/pushsync.go

+		}
+		switch dcmp {
+		case 0:
+			// do nothing


cannot happen as you returned on L371

zelig · 2020-11-18T00:12:58Z

pkg/pushsync/pushsync.go

+			closest = peer
+			return false, false, nil
+		}
+		dcmp, err := swarm.DistanceCmp(addr.Bytes(), closest.Bytes(), peer.Bytes())


I dont think this is needed. The iterator should take care of this.

I am not sure I get this comment.

I have checked (before completing the PR) about how the ClosestPeer works. It is expected that this function always goes through ALL peers, and this compare function then decides which is the "closest".

That is true. This is needed as ClosestPeer iterates on all peers, as @zbiljic described. This part was required in retrieval also, to avoid upstream requests.

oh. my. god. apologies then. our technical debt is way bigger than i thought

zelig · 2020-11-18T00:22:47Z

pkg/pushsync/pushsync.go

+	err := ps.peerSuggester.EachPeerRev(func(peer swarm.Address, po uint8) (bool, bool, error) {
+		for _, a := range skipPeers {
+			if a.Equal(peer) {
+				return false, false, nil
+			}
+		}
+		if closest.IsZero() {
+			closest = peer
+			return false, false, nil
+		}
+		dcmp, err := swarm.DistanceCmp(addr.Bytes(), closest.Bytes(), peer.Bytes())
+		if err != nil {
+			return false, false, fmt.Errorf("distance compare error. addr %s closest %s peer %s: %w", addr.String(), closest.String(), peer.String(), err)
+		}
+		switch dcmp {
+		case 0:
+			// do nothing
+		case -1:
+			// current peer is closer
+			closest = peer
+		case 1:
+			// closest is already closer to chunk
+			// do nothing
+		}
+		return false, false, nil
+	})


Suggested change

err := ps.peerSuggester.EachPeerRev(func(peer swarm.Address, po uint8) (bool, bool, error) {

for _, a := range skipPeers {

if a.Equal(peer) {

return false, false, nil

}

}

if closest.IsZero() {

closest = peer

return false, false, nil

}

dcmp, err := swarm.DistanceCmp(addr.Bytes(), closest.Bytes(), peer.Bytes())

if err != nil {

return false, false, fmt.Errorf("distance compare error. addr %s closest %s peer %s: %w", addr.String(), closest.String(), peer.String(), err)

}

switch dcmp {

case 0:

// do nothing

case -1:

// current peer is closer

closest = peer

case 1:

// closest is already closer to chunk

// do nothing

}

return false, false, nil

})

err := ps.peerSuggester.EachPeerRev(func(peer swarm.Address, po uint8) (bool, bool, error) {

if contains(skipPeers, peer) {

return false, false, nil

}

closest = peer

return true, false, nil

}

})

As noted in another comment, EachPeerer interface is sort-of misleading (also note that function used is EachPeerRev, whose comment notes that it "iterates from farthest bin to closest"), as it is expected that all peers are processed.

zelig · 2020-11-18T00:24:46Z

pkg/pushsync/pushsync.go

 	}

-	return rec, nil
+	return closest, nil
+}


this entire functionshould be part of peerSuggester

zelig · 2020-11-18T00:35:20Z

pkg/topology/topology.go

@@ -31,6 +31,11 @@ type PeerAdder interface {
 	AddPeers(ctx context.Context, addr ...swarm.Address) error
 }

+type Peerer interface {


i dont. see the need for several suggester interfaces.
the single one should be called topology.Routerand should expose a Nearest function that always takes a skip array:

type Router interface { Nearest(addr swarm.Address, skip ...swarm.Address) (peerAddr swarm.Address, err error) }

zelig · 2020-11-18T00:44:07Z

pkg/topology/mock/mock.go

 	return 0
 }

 // EachPeer iterates from closest bin to farthest
-func (_ *mock) EachPeer(_ topology.EachPeerFunc) error {
-	panic("not implemented") // TODO: Implement
+func (d *mock) EachPeer(f topology.EachPeerFunc) (err error) {


implement stop. condition if you expose this at all.
since the mock did not implement this i guess it is not used in which case only expose `ClosestPeer

` which always takes a skip list

zelig · 2020-11-18T01:00:56Z

pkg/pushsync/pushsync.go

+	return closest, nil
+}
+
+func (ps *PushSync) blocklistPeer(peer swarm.Address) {


this should be abstracted in blocklist pkg

Not sure about this, it's just separate method as its called on two places, and has specific logging depending if error or not.

if such is needed in one protocol then inanother. one too

zelig · 2020-11-18T13:55:34Z

pkg/topology/topology.go

+	// ClosestPeer returns the closest connected peer we have in relation to a
+	// given chunk address.
+	// This function will ignore peers with addresses provided in skipPeers.
+	// Returns topology.ErrWantSelf in case base is the closest to the chunk.


closest to the address

janos

Thanks, @zbiljic!

zbiljic added the in progress ongoing development , hold out with review label Nov 16, 2020

zbiljic self-assigned this Nov 16, 2020

bee-runner bot added the pull-request label Nov 16, 2020

zbiljic force-pushed the feature/pushsync-error-retry branch 2 times, most recently from 68470c7 to 3f18d96 Compare November 17, 2020 09:14

zbiljic added ready for review The PR is ready to be reviewed and removed in progress ongoing development , hold out with review labels Nov 17, 2020

zbiljic requested review from acud, janos and zelig November 17, 2020 12:15

zbiljic marked this pull request as ready for review November 17, 2020 12:15

zbiljic mentioned this pull request Nov 17, 2020

Flaky gc check #954

Closed

janos reviewed Nov 17, 2020

View reviewed changes

zelig approved these changes Nov 18, 2020

View reviewed changes

zbiljic force-pushed the feature/pushsync-error-retry branch from a67d90a to 8a8267c Compare November 18, 2020 12:08

zelig approved these changes Nov 18, 2020

View reviewed changes

zbiljic requested a review from janos November 19, 2020 09:04

zbiljic added 8 commits November 19, 2020 14:14

Pushsync should try next closes peer after error

9be795c

Blocklist unresponsive peer

8de66f0

Fix lint issues

fbb362d

Increment tag sent state before receiving receipt

d7bb541

Allow 'ClosestPeer' function to ignore peers

66b659a

Clear some state after each iteration

04ca395

Fix lint issues

af73b57

Update docs comment

9ea94bb

zbiljic force-pushed the feature/pushsync-error-retry branch from 7b63bfe to 9ea94bb Compare November 19, 2020 13:18

janos approved these changes Nov 19, 2020

View reviewed changes

zbiljic merged commit 15e1b50 into master Nov 19, 2020

zbiljic deleted the feature/pushsync-error-retry branch November 19, 2020 15:13

This was referenced Dec 8, 2020

Timeout of retrieve / pushsync #1044

Closed

Push sync cannot sync chunk #843

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pushsync retry next peer after error #947

Pushsync retry next peer after error #947

zbiljic commented Nov 16, 2020 •

edited

zbiljic commented Nov 17, 2020

zbiljic commented Nov 17, 2020

zbiljic commented Nov 17, 2020

janos left a comment

janos Nov 17, 2020

zbiljic Nov 18, 2020

janos Nov 17, 2020

zelig left a comment

zelig Nov 17, 2020

zelig Nov 17, 2020

zbiljic Nov 18, 2020

zelig Nov 18, 2020

zelig Nov 18, 2020

zelig Nov 18, 2020

zbiljic Nov 18, 2020

zelig Nov 18, 2020

zbiljic Nov 18, 2020

zelig Nov 18, 2020

zelig Nov 18, 2020

zbiljic Nov 18, 2020

janos Nov 18, 2020

zelig Nov 18, 2020

zelig Nov 18, 2020

zbiljic Nov 18, 2020

zelig Nov 18, 2020

zelig Nov 18, 2020

zelig Nov 18, 2020

zelig Nov 18, 2020

zbiljic Nov 18, 2020

zelig Nov 18, 2020

zelig Nov 18, 2020

janos left a comment

Pushsync retry next peer after error #947

Pushsync retry next peer after error #947

Conversation

zbiljic commented Nov 16, 2020 • edited

zbiljic commented Nov 17, 2020

zbiljic commented Nov 17, 2020

zbiljic commented Nov 17, 2020

janos 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

zelig 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

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

janos left a comment

Choose a reason for hiding this comment

zbiljic commented Nov 16, 2020 •

edited