Disassociate RT membership from connectivity #50

aarshkshah1992 · 2020-02-14T08:18:59Z

1. Address management is not a part of this PR & will be next.
2. DHT changes will be made once this goes in.
3. IMO, moving the RT inside go-libp2p-kad-dht is a bigger refactoring that will involve multiple discussions and should be done as a separate stream of work.

yusefnapora · 2020-02-14T14:24:35Z

table.go

 }

 // NewRoutingTable creates a new routing table with a given bucketsize, local ID, and latency tolerance.
-func NewRoutingTable(bucketsize int, localID ID, latency time.Duration, m peerstore.Metrics) *RoutingTable {
+// Passing a nil PeerValidationFnc disables periodic table cleanup.
+func NewRoutingTable(ctx context.Context, bucketsize int, localID ID, latency time.Duration, m peerstore.Metrics,


Just curious; why not make the peerValidationFnc one of the Options? We could potentially convert the other args (latency, etc) into Options as well, as long as we're changing the signature.

Using contexts to stop services is really an anti-pattern (yeah, I know we use it everywhere...). We should provide a close function.

Otherwise, the routing table could stop before the DHT (context cancellation isn't ordered).

@Stebalien Could you elaborate a bit on this ? Why is it an anti-pattern ? Also, could you direct me to some code which uses a Close function like you mention ?

@yusefnapora I've made the change.

Context cancellation is designed for requests: https://golang.org/pkg/context/.

Previous answer: Simulate DONT_HAVE for older peers ipfs/go-bitswap#248 (comment)

Lots of discussion here: Discussion: Adopting a pattern/library for managing long lived go-routines ipfs/kubo#5810

Basically, if we use contexts, everything will shutdown at the same time in a random order without taking dependencies into account. If we use explicit close methods, we can close a service and then have it close the sub-components it owns, etc.

We've had problems in the past where we'd, e.g., close the datastore before we're done using it.

@Stebalien Thanks for this. I've made the change. Also dug through the goprocess library for this & now have a pretty good idea of how it works internally.

Personally speaking I don't think using context this way is bad. goprocess is the antipattern, not context + cancellation. Additionally goprocess turns non-trivial goroutines into relatively speaking expensive operations.

bucket.go

options.go

table.go

Stebalien · 2020-02-14T21:41:32Z

table.go

 }

 // NewRoutingTable creates a new routing table with a given bucketsize, local ID, and latency tolerance.
-func NewRoutingTable(bucketsize int, localID ID, latency time.Duration, m peerstore.Metrics) *RoutingTable {
+// Passing a nil PeerValidationFnc disables periodic table cleanup.
+func NewRoutingTable(ctx context.Context, bucketsize int, localID ID, latency time.Duration, m peerstore.Metrics,


Using contexts to stop services is really an anti-pattern (yeah, I know we use it everywhere...). We should provide a close function.

Otherwise, the routing table could stop before the DHT (context cancellation isn't ordered).

table.go

cpl_replacement_cache.go

aarshkshah1992 · 2020-02-17T08:32:06Z

@yusefnapora @Stebalien Have made the changes save one, where I'd like some more information on why using contexts for service cancellation is an anti-pattern. Please take a look when you can.

aarshkshah1992 · 2020-02-18T07:09:42Z

@Stebalien I believe other than the one comment where we are waiting for @raulk , all your comments have been addressed. Let me know if you have any additional concerns.

Stebalien

This mostly looks right but I'm not awake enough at the moment to give a proper review. However, I do have a question.

bucket.go

aarshkshah1992 · 2020-02-20T08:10:33Z

@Stebalien

Should we dial missing peers concurrently albeit with some rate limiting vs dialing them serially as we are doing now to speed up the cleanup ?
Do you see any value in ALSO testing with a lower cleanup interval of say 10 seconds with just 1 peer validation each time. That one peer to be validated would be the peer we least recently heard from/have never heard from with a bias towards higher CPLs. The traffic would be less bursty/more predictable AND RT repair gets spread across over time leading to shorter feedback cycle versus a BIG BANG repair every x minutes.

aarshkshah1992 · 2020-02-24T06:00:23Z

ping @Stebalien

Stebalien · 2020-02-26T05:45:12Z

Should we dial missing peers concurrently albeit with some rate limiting vs dialing them serially as we are doing now to speed up the cleanup ?

Eh, that's probably not an issue for now. We're not going for speed here, we're trying to do a background cleanup.

I guess we could be too slow, but that's probably not the end of the world. Regardless, we should set a pretty short timeout.

Do you see any value in ALSO testing with a lower cleanup interval of say 10 seconds with just 1 peer validation each time. That one peer to be validated would be the peer we least recently heard from/have never heard from with a bias towards higher CPLs. The traffic would be less bursty/more predictable AND RT repair gets spread across over time leading to shorter feedback cycle versus a BIG BANG repair every x minutes.

Yes, but let's do that in a followup patch. The current code should be "good enough" and there are some tricky edge-cases here (eg., don't reconnect to the same overloaded peer every 10 seconds).

Stebalien

LGTM!

bucket.go

Stebalien · 2020-02-26T05:58:14Z

table.go

+					}
+					log.Infof("failed to validated candidate=%s", c)
+					// remove candidate
+					rt.HandlePeerDead(c)


Is this actually necessary? We've already removed it from the queue and it shouldn't be in the table. I guess it can't hurt.

It's necessary as we remove it from the replacement cache as part of HandlePeerDead .

Stebalien · 2020-02-26T06:04:19Z

(merge when ready)

disassociate RT membership from connectivity

ae0bcf5

aarshkshah1992 force-pushed the feat/283 branch from daa33aa to ae0bcf5 Compare February 14, 2020 09:21

aarshkshah1992 requested review from Stebalien, raulk, aschmahmann and dirkmc February 14, 2020 09:36

yusefnapora reviewed Feb 14, 2020

View reviewed changes

Stebalien reviewed Feb 14, 2020

View reviewed changes

changes as per review

1c814a5

implement idomatic RT shutdown

acf009a

do not export options

6cceecf

Stebalien reviewed Feb 20, 2020

View reviewed changes

bucket.go Outdated Show resolved Hide resolved

remove locking from buckets

5e06659

Stebalien approved these changes Feb 26, 2020

View reviewed changes

aarshkshah1992 merged commit 99fef9c into master Feb 26, 2020

aarshkshah1992 mentioned this pull request Feb 27, 2020

[Kbucket] Better address management for candidate peers libp2p/go-libp2p#808

Open

aarshkshah1992 mentioned this pull request Apr 2, 2020

Remove goprocess from Host libp2p/go-libp2p#865

Merged

aarshkshah1992 mentioned this pull request Dec 18, 2020

Disconnected context in the identify protocol libp2p/go-libp2p#1030

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disassociate RT membership from connectivity #50

Disassociate RT membership from connectivity #50

aarshkshah1992 commented Feb 14, 2020 •

edited

Loading

yusefnapora Feb 14, 2020

Stebalien Feb 14, 2020

aarshkshah1992 Feb 17, 2020

aarshkshah1992 Feb 17, 2020 •

edited

Loading

Stebalien Feb 17, 2020

aarshkshah1992 Feb 18, 2020

bonedaddy Mar 4, 2020 •

edited

Loading

Stebalien Feb 14, 2020

aarshkshah1992 commented Feb 17, 2020

aarshkshah1992 commented Feb 18, 2020 •

edited

Loading

Stebalien left a comment

aarshkshah1992 commented Feb 20, 2020 •

edited

Loading

aarshkshah1992 commented Feb 24, 2020

Stebalien commented Feb 26, 2020

Stebalien left a comment

Stebalien Feb 26, 2020

aarshkshah1992 Feb 26, 2020

Stebalien commented Feb 26, 2020

Disassociate RT membership from connectivity #50

Disassociate RT membership from connectivity #50

Conversation

aarshkshah1992 commented Feb 14, 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 Feb 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonedaddy Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Feb 17, 2020

aarshkshah1992 commented Feb 18, 2020 • edited Loading

Stebalien left a comment

Choose a reason for hiding this comment

aarshkshah1992 commented Feb 20, 2020 • edited Loading

aarshkshah1992 commented Feb 24, 2020

Stebalien commented Feb 26, 2020

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Feb 26, 2020

aarshkshah1992 commented Feb 14, 2020 •

edited

Loading

aarshkshah1992 Feb 17, 2020 •

edited

Loading

bonedaddy Mar 4, 2020 •

edited

Loading

aarshkshah1992 commented Feb 18, 2020 •

edited

Loading

aarshkshah1992 commented Feb 20, 2020 •

edited

Loading