Routing Table refactor for new management logic #66

aarshkshah1992 · 2020-03-31T13:42:33Z

For https://docs.google.com/document/d/1AZoU2FLa8ko1UWStt1kvNkBDvqTUwIRdwUvCROMBoXk

aarshkshah1992 · 2020-03-31T14:41:59Z

cpl_replacement_cache.go

@@ -1,97 +0,0 @@
-package kbucket
-
-import (


I really don't think we need all this anymore as it needlessly adds complexity and makes the code/RT state hard to reason about.

The fixLowPeers call in the DHT along with shorter intervals for refreshes(10 minutes now) should keep the RT in a good state.

aschmahmann

Overall seems reasonable, left a few comments/questions. This PR is mostly deleting code, so I'd try and keep the large changes (e.g. moving code around) to a minimum so that if @petar wants to review this it'll be easy even though he doesn't have deep familiarity with the kbucket implementation.

We can have a second PR that does other refactoring, go.mod updates, etc. and just have it based off of this one.

aschmahmann · 2020-03-31T22:07:14Z

options.go

-// Option is the Routing Table functional option type.
-type Option func(*options) error
-
-// options is a structure containing all the functional options that can be used when constructing a Routing Table.
-type options struct {
-	tableCleanup struct {
-		peerValidationFnc     PeerValidationFunc
-		peersForValidationFnc PeerSelectionFunc
-		peerValidationTimeout time.Duration
-		interval              time.Duration
-	}
-}
-
-// apply applies the given options to this option.
-func (o *options) apply(opts ...Option) error {
-	for i, opt := range opts {
-		if err := opt(o); err != nil {
-			return fmt.Errorf("routing table option %d failed: %s", i, err)
-		}
-	}
-	return nil
-}


I wouldn't toss this. Using functional arguments still tends to help us out a lot.

See #66 (comment).
As and when we need functional Options, we can add them back.

aschmahmann · 2020-03-31T22:09:10Z

options.go

-// Defaults are the default options. This option will be automatically
-// prepended to any options you pass to the Routing Table constructor.
-var Defaults = func(o *options) error {
-	o.tableCleanup.peerValidationTimeout = 30 * time.Second
-	o.tableCleanup.interval = 2 * time.Minute
-
-	// default selector function selects all peers that are in missing state.
-	o.tableCleanup.peersForValidationFnc = func(peers []PeerInfo) []PeerInfo {
-		var selectedPeers []PeerInfo
-		for _, p := range peers {
-			if p.State == PeerStateMissing {
-				selectedPeers = append(selectedPeers, p)
-			}
-		}
-		return selectedPeers
-	}
-
-	return nil
-}


wouldn't toss this either

See #66 (comment).

aschmahmann · 2020-03-31T22:14:54Z

table.go

-		return nil, err
-	}
-
+func NewRoutingTable(bucketsize int, localID ID, latency time.Duration, m peerstore.Metrics, maxLastSuccessfulOutboundThreshold float64) (*RoutingTable, error) {


Since we already have the options code and it's generally helpful to us since people don't normally touch the defaults I'd use that.

Maybe just make localID the only mandatory argument and everything else options (maybe keep metrics, but it seems reasonable for that to be an option too).

@aschmahmann I don't see the benefit of using Options here as other than latency, none of these values have meaningful defaults. What good would it do us to have all that extra code lying around?

@aarshkshah1992 your call. I find that every time we have an object missing a context or functional options we end up regretting it later since we have to break the function when we add/remove parameters.

Given that this package is really only used from go-libp2p-kad-dht to the point we've considered just merging them you're right that's probably fine for us to just change the function declaration later.

aschmahmann · 2020-03-31T22:30:32Z

table.go

+	// in that bucket with a lastSuccessfulOutboundQuery value above the maximum threshold and replace it.
+	allPeers := bucket.peers()
+	for _, pc := range allPeers {
+		if !pc.lastSuccessfulOutboundQuery.IsZero() &&


What does it mean when pc.lastSuccessfulOutboundQuery.IsZero()? Why is this being skipped, is this to try and avoid routing table instability of replacing one newly discovered peer with another?

You have the hit the nail on the head.

We are skipping it intentionally as we DO NOT want to evict peers that have been freshly added and haven't yet got a chance to be useful to us.

However, now that I think about it, a peer that's never been useful to us will stay put in the Routing Table as long as it successfully replies to queries, even with bogus replies. Do we want to evict fresh peers or maybe have a addedAtTime field for each peer so we can evict them if their lastSuccessfulOutboundQuery is Zero but they've already been around for too long ?

Yes, the IsZero isn't really going to work as is. The three options I see are:

Ignore the Zero case. This means that peers the get added to the routing table by querying us (or by being connected to us when we drop below the minimum threshold in the DHTs fixLowPeers function) can easily get rotated out unless we start using them for queries.

Have a separated addedAtTime and relevant addedAtThreshold

Just set the lastSuccessfulOutboundQuery to now when we add them to the routing table. This will give them effectively one round of catching up before we consider dropping them.

@petar I'm partial to option 3 here, WDYT?

@aarshkshah1992 @petar WDYT about in TryAdd just setting the lastSuccessfulOutboundQuery to time.Now(). We'll end up overwriting it later if we're actually doing an outbound query, otherwise if it's an inbound query we give it some time to be useful to us during a future bucket refresh/lookup.

@aschmahmann Makes complete sense to me. Have made the change.

aschmahmann · 2020-04-01T04:37:34Z

table_refresh.go

+package kbucket
+
+import (
+	"encoding/binary"
+	"fmt"
+	"math/rand"
+	"time"
+
+	"github.com/libp2p/go-libp2p-core/peer"
+
+	mh "github.com/multiformats/go-multihash"
+)


Why'd we move this all to a separate file. I trust that this is a copy-paste, but it bundling refactors with code changes like this makes it a bit harder to review and can be a bit bug prone. Has anything in this file or the corresponding tests changed that's worth noting?

@aschmahmann You are right. Apologies for doing this. The amount of code dumped into table.go was making it hard for me to go through and refactor so I moved all the refresh code in this file. But, it does make the PR hard to review.

It is a blind copy paste and the tests are passing as is. There's nothing to review/eyeball.

@petar Please ignore this file during review.

aarshkshah1992 · 2020-04-01T07:14:41Z

Hey @petar

Apologies for clubbing in some refactors along with this PR. The only files that would be of interest to you would be bucket.go AND table.go. You can ignore everything else during your review as @aschmahmann is on it.

If however, you find this too much of a hassle, please let me know and I'll just come up with a bunch of PR's for each.

aarshkshah1992 · 2020-04-02T05:06:08Z

@aschmahmann Let's ship it man ! Let's just ship it !

aschmahmann

LGTM

Routing Table refactor for new management logic

396aa61

aarshkshah1992 requested review from Stebalien and aschmahmann and removed request for Stebalien and aschmahmann March 31, 2020 13:42

aarshkshah1992 mentioned this pull request Mar 31, 2020

New RT management policy libp2p/go-libp2p-kad-dht#520

Merged

aarshkshah1992 requested review from Stebalien and aschmahmann March 31, 2020 14:39

aarshkshah1992 commented Mar 31, 2020

View reviewed changes

aarshkshah1992 added 2 commits March 31, 2020 22:58

use time threshold

d5c6eaa

float64 threshold

744750e

aschmahmann reviewed Apr 1, 2020

View reviewed changes

Stebalien mentioned this pull request Apr 1, 2020

fix: use accurate bucket logic #64

Merged

changes as per review

d761fe9

This was referenced Apr 2, 2020

Replace dead(not missing) peers & candidates #58

Closed

Periodic Ping for RT peers #68

Closed

aarshkshah1992 changed the title ~~[WIP] Routing Table refactor for new management logic~~ Routing Table refactor for new management logic Apr 2, 2020

differentiate between conns/queries

329774c

aschmahmann approved these changes Apr 2, 2020

View reviewed changes

aarshkshah1992 merged commit caabf64 into master Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Routing Table refactor for new management logic #66

Routing Table refactor for new management logic #66

aarshkshah1992 commented Mar 31, 2020

aarshkshah1992 Mar 31, 2020

aschmahmann left a comment

aschmahmann Mar 31, 2020

aarshkshah1992 Apr 1, 2020

aschmahmann Mar 31, 2020

aarshkshah1992 Apr 1, 2020

aschmahmann Mar 31, 2020

aarshkshah1992 Apr 1, 2020 •

edited

Loading

aschmahmann Apr 1, 2020

aschmahmann Mar 31, 2020

aarshkshah1992 Apr 1, 2020 •

edited

Loading

aschmahmann Apr 1, 2020

aschmahmann Apr 1, 2020

aarshkshah1992 Apr 2, 2020

aschmahmann Apr 1, 2020

aarshkshah1992 Apr 1, 2020 •

edited

Loading

aarshkshah1992 Apr 1, 2020

aarshkshah1992 commented Apr 1, 2020

aarshkshah1992 commented Apr 2, 2020

aschmahmann left a comment

Routing Table refactor for new management logic #66

Routing Table refactor for new management logic #66

Conversation

aarshkshah1992 commented Mar 31, 2020

Choose a reason for hiding this comment

aschmahmann 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

aarshkshah1992 Apr 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 Apr 1, 2020 • 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

aarshkshah1992 Apr 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Apr 1, 2020

aarshkshah1992 commented Apr 2, 2020

aschmahmann left a comment

Choose a reason for hiding this comment

aarshkshah1992 Apr 1, 2020 •

edited

Loading

aarshkshah1992 Apr 1, 2020 •

edited

Loading

aarshkshah1992 Apr 1, 2020 •

edited

Loading